Zipping broken #79

Closed
silverwind opened this Issue Mar 6, 2014 · 33 comments

Comments

Projects
None yet
3 participants
@silverwind
Owner

silverwind commented Mar 6, 2014

Looking into it.

@silverwind silverwind added Bug labels Mar 6, 2014

@silverwind silverwind self-assigned this Mar 6, 2014

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Mar 6, 2014

Owner

That'll teach me not to blindly update deps 😢

archiverjs/node-archiver@d8021ad

Owner

silverwind commented Mar 6, 2014

That'll teach me not to blindly update deps 😢

archiverjs/node-archiver@d8021ad

@silverwind silverwind closed this in 54b8447 Mar 6, 2014

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Mar 31, 2014

@silverwind was just browsing projects using archiver. sorry about this. i try to refrain from such breaking changes (even with semver) but sometimes needed to get to point b.

@silverwind was just browsing projects using archiver. sorry about this. i try to refrain from such breaking changes (even with semver) but sometimes needed to get to point b.

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Mar 31, 2014

also, you may be able to replace utils.walkDirectory with bulk() using the below or something like it. It will filter it to files only internally and act similar to file():

archive.bulk([
  { expand: true, cwd: zipPath, src: ['**'] }
]);

https://github.com/ctalkington/node-archiver#bulkmappings

also, you may be able to replace utils.walkDirectory with bulk() using the below or something like it. It will filter it to files only internally and act similar to file():

archive.bulk([
  { expand: true, cwd: zipPath, src: ['**'] }
]);

https://github.com/ctalkington/node-archiver#bulkmappings

@colelawrence

This comment has been minimized.

Show comment
Hide comment
@colelawrence

colelawrence Mar 31, 2014

Collaborator

Looks cool @ctalkington

Collaborator

colelawrence commented Mar 31, 2014

Looks cool @ctalkington

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Thank, I really appreciate your feedback.

I was looking at .bulk before, but wasn't sure which globbing pattern to use, I'll try yours.

The breakage was unexpected, but totally my fault, as I ran npm-check-updates without looking up the module changes. Now, I still run the update check before each release, but I'm reading the commit log or the (rare) changelog of modules now.

Owner

silverwind commented Apr 1, 2014

Thank, I really appreciate your feedback.

I was looking at .bulk before, but wasn't sure which globbing pattern to use, I'll try yours.

The breakage was unexpected, but totally my fault, as I ran npm-check-updates without looking up the module changes. Now, I still run the update check before each release, but I'm reading the commit log or the (rare) changelog of modules now.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

@ctalkington Seems to work nicely so far, only caveat I found is, that when zipping an empty directory, it won't get included in the zip. I'm not sure I can fix this on my side, could you check that out?

Owner

silverwind commented Apr 1, 2014

@ctalkington Seems to work nicely so far, only caveat I found is, that when zipping an empty directory, it won't get included in the zip. I'm not sure I can fix this on my side, could you check that out?

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

can you elaborate? im guessing you mean that zipPath contains no files so bulk doesnt run, producing an empty zip?

can you elaborate? im guessing you mean that zipPath contains no files so bulk doesnt run, producing an empty zip?

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner
archive.bulk([{ expand: true, cwd: zipPath, src: ["**"], dest: path.basename(zipPath)}]);

Assuming zipPath is /path/to/folder, dest resolves to folder, which is empty. I explicitly want to include the zipped folder in the zip itself, so there are no bad suprises with files spilling into the directory when unzipping.

I think .bulk() still runs but all I get is a 22 byte, emtpy zip file.

Owner

silverwind commented Apr 1, 2014

archive.bulk([{ expand: true, cwd: zipPath, src: ["**"], dest: path.basename(zipPath)}]);

Assuming zipPath is /path/to/folder, dest resolves to folder, which is empty. I explicitly want to include the zipped folder in the zip itself, so there are no bad suprises with files spilling into the directory when unzipping.

I think .bulk() still runs but all I get is a 22 byte, emtpy zip file.

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

i think the best thing i can recommend is going to be doing a manual directory entry just before bulk since the way those bulk are run across and such. that said i need to update node-zip-stream support directories. i will work to get this out asap as I see the value in it.

i think the best thing i can recommend is going to be doing a manual directory entry just before bulk since the way those bulk are run across and such. that said i need to update node-zip-stream support directories. i will work to get this out asap as I see the value in it.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Manually adding the root directory could work, but what if it encounters an empty directory somewhere deeper in the path? I'll eagerly await your updates :)

Owner

silverwind commented Apr 1, 2014

Manually adding the root directory could work, but what if it encounters an empty directory somewhere deeper in the path? I'll eagerly await your updates :)

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

the code would end up being something like (once proper directory support is there):

archive.entry(null, { name: path.basename(zipPath), type: 'directory' });

the globbing itself does pickup the dirs, just filtered out internally atm since writing every directory when it has a file is a bit overfill imho. what i really want to do here is:

a. support type: directory
b. when bulk gets a directory, add it to a list that then is filtered to see if another file path is inside it. and add all the empty dirs at end of archive.

the code would end up being something like (once proper directory support is there):

archive.entry(null, { name: path.basename(zipPath), type: 'directory' });

the globbing itself does pickup the dirs, just filtered out internally atm since writing every directory when it has a file is a bit overfill imho. what i really want to do here is:

a. support type: directory
b. when bulk gets a directory, add it to a list that then is filtered to see if another file path is inside it. and add all the empty dirs at end of archive.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Sounds great!

Owner

silverwind commented Apr 1, 2014

Sounds great!

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

should be roll out an update today. just working out some aspects on both archiver and zip-stream

should be roll out an update today. just working out some aspects on both archiver and zip-stream

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

@silverwind give archiver v0.8.0 a go ;)

@silverwind give archiver v0.8.0 a go ;)

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

also it would be the following to enforce a folder inside even if source dir is empty:

archive.append(null, { name: path.basename(zipPath) + '/' });

also it would be the following to enforce a folder inside even if source dir is empty:

archive.append(null, { name: path.basename(zipPath) + '/' });
@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

I'm a bit confused as to where to put the absolute path to the directory. Is archiver.entry the correct function?

archive.entry(zipPath, { name: path.basename(zipPath), type: 'directory' });

The above gave me Object [object Object] has no method 'entry', thought it may just be my npm acting up, I get all kind of strange JSON errors when installing dependencies right now.

Owner

silverwind commented Apr 1, 2014

I'm a bit confused as to where to put the absolute path to the directory. Is archiver.entry the correct function?

archive.entry(zipPath, { name: path.basename(zipPath), type: 'directory' });

The above gave me Object [object Object] has no method 'entry', thought it may just be my npm acting up, I get all kind of strange JSON errors when installing dependencies right now.

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

yah entry is zip-stream. you'd want append for archiver. sorry about that.

yah entry is zip-stream. you'd want append for archiver. sorry about that.

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

should look like this i think:

archive.append(null, { name: path.basename(zipPath) + '/' });
archive.bulk(...);

should look like this i think:

archive.append(null, { name: path.basename(zipPath) + '/' });
archive.bulk(...);
@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Oh, alright. Didn't realize you reworked .bulk after all.

Owner

silverwind commented Apr 1, 2014

Oh, alright. Didn't realize you reworked .bulk after all.

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

yes, the only thing the recent changes don't do is filter out redundant directory entries, i figured best to have the little extra size and ability to add empty dirs for now. will revisit for optimization in future release.

yes, the only thing the recent changes don't do is filter out redundant directory entries, i figured best to have the little extra size and ability to add empty dirs for now. will revisit for optimization in future release.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Looking good so far. I noticed something, that might be a bug:

archive.bulk([{ expand: true, cwd: zipPath, src: ["**"]}]);

Running the above (note, no dest) on an empty dir gives an zip with an . file in it. WinRAR thinks it's an "Local Disk".

Owner

silverwind commented Apr 1, 2014

Looking good so far. I noticed something, that might be a bug:

archive.bulk([{ expand: true, cwd: zipPath, src: ["**"]}]);

Running the above (note, no dest) on an empty dir gives an zip with an . file in it. WinRAR thinks it's an "Local Disk".

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

rar

Owner

silverwind commented Apr 1, 2014

rar

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

thats most likely more to do with the expand logic from grunt, which expects a dest to be set.

EDIT: hum actually, it may be something else as i've used without dest before.

thats most likely more to do with the expand logic from grunt, which expects a dest to be set.

EDIT: hum actually, it may be something else as i've used without dest before.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Works flawlessly with a dest thought, thanks again!

Owner

silverwind commented Apr 1, 2014

Works flawlessly with a dest thought, thanks again!

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Should I open an issue at archiver, or do you wanna fix it on the fly?

Owner

silverwind commented Apr 1, 2014

Should I open an issue at archiver, or do you wanna fix it on the fly?

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

if you want to reg an issue that would be good. ill have to write a test or two and try and narrow why it happens. initial testing appears fine locally. what OS do you use, something similar on windows returns empty array which shouldn't cause anything to be added.

EDIT: caught it in action. feel free to file the issue though.

if you want to reg an issue that would be good. ill have to write a test or two and try and narrow why it happens. initial testing appears fine locally. what OS do you use, something similar on windows returns empty array which shouldn't cause anything to be added.

EDIT: caught it in action. feel free to file the issue though.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

This test was on Windows, will test on other platforms and then do an issue.

Owner

silverwind commented Apr 1, 2014

This test was on Windows, will test on other platforms and then do an issue.

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

see archiverjs/node-archiver@d684de8. thanks for the testing. its just the way glob handles ** in cwd.

see archiverjs/node-archiver@d684de8. thanks for the testing. its just the way glob handles ** in cwd.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Ah, I' check this out. I just finished testing on OS X and it showed me ".-1" as an errounous entry with a client on the same machine. Though I used a different zip utility over there.

Owner

silverwind commented Apr 1, 2014

Ah, I' check this out. I just finished testing on OS X and it showed me ".-1" as an errounous entry with a client on the same machine. Though I used a different zip utility over there.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

Looks good now. I get the expected 22 byte zip files without contents.

Owner

silverwind commented Apr 1, 2014

Looks good now. I get the expected 22 byte zip files without contents.

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

@silverwind glad to hear, do let me know if you run into anything in the future. i try to push out fixes asap.

@silverwind glad to hear, do let me know if you run into anything in the future. i try to push out fixes asap.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 1, 2014

Owner

If you could do a patch release, that would be great :)

Edit: nevermind, I see you did already.

Owner

silverwind commented Apr 1, 2014

If you could do a patch release, that would be great :)

Edit: nevermind, I see you did already.

@ctalkington

This comment has been minimized.

Show comment
Hide comment
@ctalkington

ctalkington Apr 1, 2014

already done, npm is just a tad slow.

already done, npm is just a tad slow.

silverwind added a commit that referenced this issue Apr 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment