-
-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix creating unusable ZIP files on Windows #117
Conversation
From what I understand, the root of this issue is that mode bits on directories in zip files are platform-specific, usually .zip files contain no directory information, only files, or all directories have a mode of 0, which leaves it up to the platform to decide what to do. Just passing a mode of 0 along for the directory entries would probably also be a perfectly valid fix. |
index.js
Outdated
mode: file.stat.mode | ||
// Set executable bit on directories if any other bits are set for that user/group/all | ||
// Fixes creating unusable zip files on platforms that do not use an executable bit | ||
mode: file.stat.mode | (((file.stat.mode >> 1) | (file.stat.mode >> 2)) & 0o111) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be fixed in https://github.com/gulpjs/vinyl-fs ? This plugin just passes the .mode
it gets from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, or actually, this seems like an issue in https://github.com/thejoshwolfe/yazl ? Since it should correctly handle a normal mode and do the right thing depending on the platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps? YAZL is already exposing addEmptyDirectory
as something special - empty directories in zip files are only cross-platform if you use a default mode of 0, as far as I understand it, so maybe should just always passing 0
here / leaving off the optional mode field would be fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be very weird for YAZL to do anything with the 'mode' bits passed to it other than store it directly in the zip file (for people using zips on a single environment that's sometimes important). Presumably most people are using gulp-zip
as a part of build and distribution pipelines, where the output is expected to be cross-platform / just work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just feel that YAZL should not allow creating ZIPs that end up being invalid on certain platforms. It would be better if YAZL either threw and error or did the right thing. Don't you agree?
As for fixing this here, I'm fine with just passing 0
.
where the output is expected to be cross-platform / just work.
I cannot imagine any real scenario where you wouldn't want it to be cross-platform. You never know where the ZIP will eventually be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception is when you're doing system maintenance kind of stuff - if I make a zip of my user directory, I would, perhaps, expect it to save the platform-specific mode bits and restore them when I unzip them. But, since it doesn't include other platform-specific stuff like owner and group or security ACLs, it'd be a horrible choice for anything maintenance related, so maybe you're right and it just shouldn't be that way, but that seems to be the way zip is. If you make a zip on Windows with the built-in PowerShell zip function, it also makes incompatible zip files in the same way :(. This really seems like a bug in the unzipping side, but that's quite far out of our control =).
Upon consideration, I agree my earlier arguments about cross-platform should apply to YAZL, even if I also think that would be weird - I'll send the same PR setting the executable bits inside of addEmptyDirectory()
to YAZL and see how that maintainer feels =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent a PR over to YAZL, but it's been silent so far. Sounds like YAZL is only semi-maintained, so might take some time before it gets resolved or rejected.
Well, it's been a month and a half, and zero activity from the maintainer of YAZL, so I don't expect the PR over there will be merged any time soon, if ever... Want to fix this here, or should we make a fork of YAZL with bugfixes and depend on that instead? |
I’m ok with fixing it here. Can you add a code comment with a link to the YAZL issue? I’m case it ever gets fixed and we can remove the workaround. |
Sure, can do. Do you prefer the change as-written (adding the executable bit) or just passing |
I would prefer passing |
This allows directories to be crated with their default platform-appropriate mode (0775).
Okay, have committed the simpler fix. Two notes:
|
Fixes #112
Fixes #106
Fixes #76
IssueHunt Summary
Referenced issues
This pull request has been submitted to: