bugGNU Octave - Bugs: bug #47609, zip.m: errrors when zipping to a...

 
 

bug #47609: zip.m: errrors when zipping to a directory with spaces in name on Windows

Submitter:  Philip Nienhuis <philipnienhuis>
Submitted:  Sat 02 Apr 2016 10:40:23 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Philip Nienhuis Open/Closed:  * Closed
Release:  * dev Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 04 Apr 2016 05:34:32 PM UTC, comment #12: 

I agree, this will probably confuse some Windows users and should probably be mentioned someplace, I don't know what the right place is :)

Closing this one as fixed.

Mike Miller <mtmiller>
Group Member
Mon 04 Apr 2016 05:21:07 PM UTC, comment #11: 

@comment #10:
Sure, but where?
In the various texinfo headers?
In the manual?
On the wiki? (FAQ?)

One might argue that as this wildcard behavior is a MingW/MSYS issue it had better be outlined in a general place.

My point is merely that most Windows users probably do not expect *nix behavior on a Windows box. They're probably not even aware that wildcard treatment is different from other, "normal" Windows programs.
Too bad I haven't got the experimental MSVC binaries lying around that Michael Goffioul once made, to check how this worked with an Octave built with native Windows tools.
It's worthwile to check with Matlab as well; must happen at work as the prerelease timed out.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 04 Apr 2016 04:29:48 PM UTC, comment #10: 

Just pushed:

http://hg.savannah.gnu.org/hgweb/octave/rev/3b9524234d89

Want to tackle the documentation issues of how to teach Windows users how wildcards work in a separate bug report?

Mike Miller <mtmiller>
Group Member
Mon 04 Apr 2016 04:26:36 PM UTC, comment #9: 

@Philip: Yes, cygwin is very annoying in that it returns true for both ispc and isunix.  A true Windows machine is


windows = ispc () && ! isunix ();



Rik <rik5>
Group administrator
Mon 04 Apr 2016 04:00:23 PM UTC, comment #8: 

Is the "! isunix ()" required for avoiding cygwin? (i.e., do ispc() and isunix() both return true on cygwin?)

Anyway the patch looks fine to me. Will you push or shall I?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 03 Apr 2016 08:41:46 PM UTC, comment #7: 

Right, setting the documentation of wildcards aside for now, how about this revision of your patch?

(file #36829)

Mike Miller <mtmiller>
Group Member
Sun 03 Apr 2016 07:06:57 PM UTC, comment #6: 

As far as my experience with MSYS/mingw goes, yes this wildcard "issue" (idiosyncrasy) is valid for all utilities. So probably unpack / gzip / glob / dir could also use relevant information in the texinfo header. See below for dir and glob:

>> numel (dir ("*.*"))
ans =  104
>> numel (dir ("*"))
ans =  124
>> numel (glob ("*.*"))
ans =  104
>> numel (glob ("*"))
ans =  122  ## glob() doesn't catch "." and ".." so 2 less than dir()


ls seems not affected:

>> ls *
 Volume in drive C has no label.
 Volume Serial Number is 5E8A-7D68

 Directory of C:\Users\philip\MyDocs\octave
:
             105 File(s)     29 146 282 bytes
              19 Dir(s)  88 849 948 672 bytes free

-and-

>> ls *.*
:
             105 File(s)     29 146 282 bytes
              19 Dir(s)  88 849 948 672 bytes free


But then one gets the question whether this should be mentioned in those texinfo headers, or in a more general place in the manual (with more chance of being overlooked by users), and/or in the FAQ ?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 03 Apr 2016 05:26:28 PM UTC, comment #5: 

I agree that it would be better to not "fix" the wildcard automatically but document it. My main question is, doesn't this affect every command that accepts wildcards? Such as dir, ls, glob, gzip, zip, unpack?

My point in comment #2 was because I got confused with all the permutations. Indicating the path separately works because Octave knows how to chdir to that path and the zip command with argument "*" then succeeds. I thought I had seen you post an example that worked with a "backslashed\path\*" argument, but I was wrong. So yes, I agree this seems like a bug with the zip executable that we should work around as you indicated.

The condition should be


if (ispc () and ! isunix ())


as used elsewhere in scripts.

Mike Miller <mtmiller>
Group Member
Sun 03 Apr 2016 04:55:48 PM UTC, comment #4: 

@comment #3:
*.* on Windows is usually meant to catch all files and subdirs, with or without extension (on Windows files and directories can have extensions; extensions of directories are not special, just part of a name).
That works differently on *nix - the period between file/directory name and extension isn't special on *nix (and msys/mingw and cygwin), while it is on Windows (that is, Windows utilities and -programs find it special => Windows users perceive it as special too).
I found that *.* even works adversely on mingw, see the first two verbatim blocks in comment #1, hence my advice in the texinfo header in the changeset.

Of course zip.m can have stanza along the lines of:

if (ispc && strcmp (files, "*.*"))
  files = "*";
endif


but I think it is better to have users know what they're doing than have programs guessing user's intentions. After all, it may be possible that a literal *.* is required and/or intended.


@comment #2:
The cmd lines obtained in zip.m that are conveyed to zip.exe are in the 5th verbatim block of comment #1.

The purpose of my cset is simply to ensure properly zipping to directories with spaces in the name using zip.m on Windows systems.
On Linux, zip.m and friends work as advertised, probably due to Rik's recent work.

Backslashes give problems when fed to zip(.exe), see the 5th verbatim block in comment #1 (debugging zip.m on Windows).
Several mingw utilities behave inconsistently as regards file separator styles.  zip.exe is a fine example: it does not error on backslashes in its first argument (the zip file) but does in the second (the files to be zipped). I did not include that in the verbatim block, but that's what I got while experimenting.

One can argue that backslash behavior had rather be fixed upstream, but I think for now it is vastly better to simply avoid backslash fileseps at all when dealing with mingw utilities.  All of those those work fine with forward slash fileseps.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 03 Apr 2016 02:57:57 PM UTC, comment #3: 

Is the


*.*


wildcard typically expected to match directory names with no extension on Windows? What about files with no extension?

Mike Miller <mtmiller>
Group Member
Sun 03 Apr 2016 02:56:52 PM UTC, comment #2: 

Is the . wildcard expected to match directory names with no extension in Windows typically? That seems like a more generic problem/misunderstanding not specific to zip.

I got some errors due to paths with spaces in Debian with 4.0.1, but not with the default branch:


>> out = zip ("dir_no_spaces/foo1.zip", "file*", "dir_no_spaces");
out =
{
  [1,1] = file1
  [1,2] = file2
  [1,3] = file3
}
>> out = zip ("dir_no_spaces/foo2.zip", "dir_no_spaces/file*");
out =
{
  [1,1] = dir_no_spaces/file1
  [1,2] = dir_no_spaces/file2
  [1,3] = dir_no_spaces/file3
}
>> out = zip ("dir with spaces/foo3.zip", "file*", "dir with spaces");
out =
{
  [1,1] = file1
  [1,2] = file2
  [1,3] = file3
}
>> out = zip ("dir with spaces/foo4.zip", "dir with spaces/file*");
out =
{
  [1,1] = dir with spaces/file1
  [1,2] = dir with spaces/file2
  [1,3] = dir with spaces/file3
}


So no errors due to spaces here, and no errors due to spaces in Philip's examples either, just errors due to backslash file separator.

That's suspicious that the same path results in no error in the previous example. How is that working? What is cmd in the case where it does work and why does that work and not the other?

Mike Miller <mtmiller>
Group Member
Sat 02 Apr 2016 10:53:37 PM UTC, comment #1: 

It seems the bugreport didn't get uploaded in the form I hoped it would be :-(

Another try:

Files to zip are in:
C:\Users\philip\AppData\Local\Temp\oct-dIRfU3

File to zip to:
c:\tmp\spaces in name\test.zip


flist = zip ("test.zip", "*.*", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3')


Works partly. dir apparently does not understand the traditional Windows wildcard ..
It only copied the one file in the root of ....\oct-dIRfU3 but not the subdirs


flist = zip ("test.zip", "*", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3')


This ^^ works fine.


flist = zip ("spaces in name/test.zip", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3\*')
error: zip: zip failed with exit status = 127
error: called from
    zip at line 72 column 5


Does not work at all.


flist = zip ("spaces in name/test.zip", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3\*.*')
flist = zip ("spaces in name/test.zip", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3\')


Same, don't work.

Debugging zip.m:

## from within c:\tmp\spaces in name\
flist = zip ("test.zip", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3\*')
:
## At L.68
debug> cmd
cmd = zip -r 'C:\tmp\spaces in name\test.zip'  C:\Users\philip\AppData\Local\Temp
\oct-dIRfU3\*
debug> [status, output] = system (cmd)  ## Run from editor with F9

status =  127
output =        zip warning: name not matched: C:/Users/philip/AppData/Local/Temp
/oct-dIRfU3/*

zip error: Nothing to do! (try: zip -r C:\tmp\spaces in name\test.zip . -i C:\Use
rs\philip\AppData\Local\Temp\oct-dIRfU3\*)
:
## Changing backslashes to forward slashes
debug> cmd = strrep (cmd, '\', '/')
cmd = zip -r 'C:/tmp/spaces in name/test.zip'  C:/Users/philip/AppData/Local/Temp
/oct-dIRfU3/*
debug> [status, output] = system (cmd)

Local/Temp/oct-dIRfU3/[Content_Types].xml (deflated 69%)
p/oct-dIRfU3/_rels/ (stored 0%)
p/oct-dIRfU3/_rels/.rels (deflated 60%)
p/oct-dIRfU3/docProps/ (stored 0%)
p/oct-dIRfU3/docProps/app.xml (deflated 51%)
:


Conclusions:
- In zip.m, Windows users should be informed that "*.*" wildcard doesn't work as expected on Windows, they should use "*" instead;
- Ditto, the FILES argument should have forward slash fileseps

Changeset for these two attached.


(file #36825)

Philip Nienhuis <philipnienhuis>
Group Member
Sat 02 Apr 2016 10:40:23 PM UTC, original submission:  

Some investigation after bug #47578 shows the following:

Files to zip are in:
C:\Users\philip\AppData\Local\Temp\oct-dIRfU3

File to zip to:
c:\tmp\spaces in name\test.zip


flist = zip ("test.zip", "*.*", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3')
## Works partly. zip apparently does not understand the traditional Windows wildcard *.*.
## It only copied the one file in the root of ....\oct-dIRfU3 but not the subdirs



flist = zip ("test.zip", "*", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3')
## This works fine



flist = zip ("spaces in name/test.zip", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3\*')
## Does not work at all:
error: zip: zip failed with exit status = 127
error: called from
    zip at line 72 column 5



flist = zip ("spaces in name/test.zip", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3\*.*')
## Same, does not work



flist = zip ("spaces in name/test.zip", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3\')
## Same, does not work

Debugging zip.m:
+verbatim+
## from within c:\tmp\spaces in name\
flist = zip ("test.zip", 'C:\Users\philip\AppData\Local\Temp\oct-dIRfU3\*')
:
## At L.68
debug> cmd
cmd = zip -r 'C:\tmp\spaces in name\test.zip'  C:\Users\philip\AppData\Local\Temp
\oct-dIRfU3\*
debug> [status, output] = system (cmd)  ## Run from editor with F9

status =  127
output =        zip warning: name not matched: C:/Users/philip/AppData/Local/Temp
/oct-dIRfU3/*

zip error: Nothing to do! (try: zip -r C:\tmp\spaces in name\test.zip . -i C:\Use
rs\philip\AppData\Local\Temp\oct-dIRfU3\*)
:
:
## Changing backslashes to forward slashes
debug> cmd = strrep (cmd, '\', '/')
cmd = zip -r 'C:/tmp/spaces in name/test.zip'  C:/Users/philip/AppData/Local/Temp
/oct-dIRfU3/*
debug> [status, output] = system (cmd)

Local/Temp/oct-dIRfU3/[Content_Types].xml (deflated 69%)
p/oct-dIRfU3/_rels/ (stored 0%)
p/oct-dIRfU3/_rels/.rels (deflated 60%)
p/oct-dIRfU3/docProps/ (stored 0%)
p/oct-dIRfU3/docProps/app.xml (deflated 51%)
:


Conclusions:

- In zip.m, Windows users should be informed that the "*.*" wildcard doesn't work as expected on Windows, they should use "*" instead;

- Ditto, the FILES argument should have forward slash fileseps; or zip.m itself should take care that it conveys forward slash fileseps to zip.

I'll upload a changeset to accomplish both these points when I have the bug number.

Philip Nienhuis <philipnienhuis>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #36829:  bug-47609-2.diff added by mtmiller (947B - text/x-diff)
file #36825:  bug_47609.cset added by philipnienhuis (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by philipnienhuis (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-04-04 mtmiller StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-04-03 mtmiller Attached File- Added bug-47609-2.diff, #36829
        StatusNone Patch Submitted
    2016-04-02 philipnienhuis Attached File- Added bug_47609.cset, #36825

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code