sindresorhus / gulp-zip

ZIP compress files
MIT License
270 stars 47 forks source link

755 folder permissions stomped on my lambda... #64

Closed jjmartin closed 8 years ago

jjmartin commented 8 years ago

https://github.com/sindresorhus/gulp-zip/issues/62

This fix (i think) broke my AWS lambda function. With 3.0.2 it works fine but with 3.1.0, the AWS lambda isn't able to load external modules. (it could be something else but we have explicitly tested with the only change being the version number of gulp-zip)

I'm not saying this is a bug in gulp-zip - but i would like to know if there are options to set it back to the permissions i think were working. if you think it was some other change between 3.0.2 and 3.1.0, i'd be glad to explore that too.

Thanks for your help.

sindresorhus commented 8 years ago

It was just changed to correctly preserve directory permissions: https://github.com/sindresorhus/gulp-zip/commit/864a8fd321673b2413f479610fb3a763f0e4e680

pe8ter commented 8 years ago

Are you saying you don't believe this to be a bug?

Edit: I see what's happening. Before this update, the bad permissions just happened to work for our project. After the update, the permissions were being set correctly and those correct permissions were breaking our app.

t1st3 commented 8 years ago

@jjmartin @pe8ter I just tested on a Fedora Linux with various sets of permissions and multiple nested files and folders, and I can't reproduce the problem. All the permissions are kept in the zip file as they were before using the plugin.

t1st3 commented 8 years ago

@pe8ter so, to you, this issue may be closed?

pe8ter commented 8 years ago

Yes, but it's not my Issue to say so. Thanks for timely responses!

t1st3 commented 8 years ago

@pe8ter you're welcome! Sure, the issue is not yours, but yet, 1 of 2 issuers are OK with the module's behavior!

jjmartin commented 8 years ago

maybe it wasn't the permissions on ours that was causing the problem - all the issues we had with our deployment were coming from windows - they all work correctly with 3.0.2 but failed with 3.1.0...

we aren't setting any special permissions on the folders, its a newly created folder that gets zipped. and its hard to tell exactly what the problem is from the AWS lambda side, we have re-downloaded the zip from amazon and not had any problems opening it or running the node scripts inside of them.

pe8ter commented 8 years ago

@jjmartin We were building on a Windows box too. Time to update!

jjmartin commented 8 years ago

time to update what tho - i am not sure what i need to be doing differently... i can accept that we are doing something wrong but its pretty default behavior...

gulp.task('zip', ['npm', 'subfolders'], function () {    
    return gulp.src([distributionFolder + '**/*'])    
        .pipe(zip(butils.getZipName()))    
        .pipe(gulp.dest(distributionFolder));    
});
pe8ter commented 8 years ago

This update means it's time for us to not build on a Windows box and deploy to a Linix machine. Hard to say much else without details of your CI.

jjmartin commented 8 years ago

well that is unacceptable... one of the whole points of my migrating my deployment to gulp was so that it would be cross platform. (was previously using a powershell script).

My build server actually is linux and i believe i still get the issue.

t1st3 commented 8 years ago

@jjmartin After a few new tests, I could reproduce your problem.

I assume your variable distributionFolder is a folder path, like this one my-folder/my-sub, without any slash at the end, so the glob you're having looks like this: my-folder/my-sub**/*. Using this glob, you'll get permission errors (I don't know if it's intended, or if it's a bug).

Try to add a slash after the folder name, like this one my-folder/my-sub/**/*, and you'll get a ZIP without any permission errors (but you will lose the top-level folder my-sub in that ZIP).

jjmartin commented 8 years ago

ok thanks - i will give that a try ... that seems like a workaround or fix that i can live with.

jjmartin commented 8 years ago

actually after getting a chance to look back at my code that distribution folder does have a trailing slash so it looks somthing like '../myproject-dist/' and the final glob looks like '../myproject-dist/*/' which i think is about what you are saying it should look like, but we did get the issue of Lambda not being able to read the node_modules folder.

is there some windows setting i can put on the folders as i create them that would make the gulp-zip 3.1.0 make the folders 775 again? (or appear a such to the linux container that lambda runs in? )

jjmartin commented 8 years ago

ok after watching a debug and reading a bunch - i think i understand the underlying problem. Windows is ALWAYS going to return 16822 for its stat.mode on a directory. when translated to octal we get 40666 - (directory)read/write for owner, group and others (no execute)

I'm assuming that the node_modules of my deployment should have execute permissions (otherwise i wouldn't be having this problem)

would you accept a merge request that included an option to force set a mode on the zipped files/folders? (or left them unset)

i think thats the only solution that i would be able to continue to use gulp-zip to do this - i think i'd rather move to my own use of yazl or another library rather than remained pinned to a specific version.

sindresorhus commented 8 years ago

@jjmartin yazl seems to default to 040775 for directories, so not sure why gulp differs. This should really be brought up on gulp issue tracker, and not here. All this plugin does is pass the values from gulp to yazl. Not interested in having an option. You can use https://github.com/sindresorhus/gulp-chown for that.

jjmartin commented 8 years ago

ok - i'll look into the gulp-chown - that is totally a fine solution -

yazl did do 775 by default (this is what was set in 3.0.2), but since you added your own mode setting - you aren't using the default anymore - you are using what is coming off of the node file.stat.mode which in this case of folders is the 16822.

jjmartin commented 8 years ago

just in case anyone else is finding this - i think what i actually want is your other project https://github.com/sindresorhus/gulp-chmod

timdp commented 8 years ago

We ran into a related issue, which I believe might be the underlying cause of some of the symptoms described in this thread. I just wanted to document it here.

We're creating zip files on Windows and unzipping them on Linux. (Actually, AWS Elastic Beanstalk is doing the latter for us.) The code that creates the zip files used to be fairly trivial:

return gulp.src('build/**/*')
  .pipe(zip('package.zip'))
  .pipe(gulp.dest('dist'))

The thing is that the glob also matches directories, which makes gulp-zip create explicit directory entries in the zip file, copying their permissions. Unlike Linux, Windows doesn't require that directories be executable in order to allow access, meaning that the directory entries in the zip file won't be marked as executable. Consequently, unzipping package.zip on Linux will render every subdirectory inaccessible. We verified this by opening the archive in 7-Zip File Manager and checking the Attributes column, but there are of course many other ways.

The easiest fix we could think of is passing {nodir: true} to gulp.src:

return gulp.src('dist/**/*', {nodir: true})

This will only pass files to gulp-zip, keeping it from creating explicit directory entries, and instead creating the directories implicitly upon unzipping files contained inside them. Of course, this means that directory permissions won't be preserved, which may not be what you're after. (I didn't check, but I'm pretty sure the default directory permissions from umask will be applied in this case.) Also, empty directories won't be created, but you can easily work around that with an empty file.

Of course, as Sindre already mentioned, gulp-chmod and/or gulp-chown will work as well, but you'll need to combined it with gulp-filter to only modify directories, which could become tedious. Alternatively, I guess you could also pipe the files through an object stream yourself, check if they're directories, and change their permissions.

sindresorhus commented 8 years ago

Open an issue/PR on https://github.com/gulpjs/vinyl-fs. This plugin is just passing the permissions it get's from there.

timdp commented 8 years ago

@sindresorhus Do you think that's relevant though? I don't think vinyl-fs is doing something wrong per se. It's just that the permissions on Windows and Linux don't align, so it kinda makes sense that you have to do some additional work if you really want to preserve them.

timdp commented 8 years ago

By which I don't mean that gulp-zip should take care of that for you. An option like autoMakeDirectoriesExecutable would probably save a lot of people a bit of code, but it's far from generic and it opens a can of woms.

jjmartin commented 8 years ago

just as a followup for people finding this... this was my workaround: using the gulp-tap:

gulp.task('zip', ['npm', 'subfolders'], function() {
    return gulp.src([distributionFolder + '**/*'])
        .pipe(tap(function(file) {
            if (file.isDirectory()) {
                file.stat.mode = parseInt('40777', 8);
            }
        }))
        .pipe(zip(distroZip))
        .pipe(gulp.dest(distributionFolder));
});
softwaredude commented 8 years ago

As another follow-up for those finding this: Thank you, @timdp! Your idea to use the {nodir: true} option to gulp.src worked for me in my situation of using gulp-zip to create a PhoneGap Build package on Windows and having the upload to PGB fail in weird, nondeterministic ways.

orangewise commented 8 years ago

@jjmartin, your tap workaround saved my day!

mbruning24 commented 8 years ago

@jjmartin 3.5 hours of struggling with this today and BOOM your gulp-tap solution works. I REALLY need to get a linux machine so I don't have to deal with windows-related issues...

jmahmud commented 8 years ago

This was a great suggestion @jjmartin !! Many thanks for the workaround.

gpierrick commented 8 years ago

@jjmartin not sure what your tap workaround does but it saved me from hours of figuring it out! thanks

tstanev commented 7 years ago

@timdp suggestion to add {nodir: true} also works when deploying a zip from Windows to AWS Lambda. Without it, Lambda has a problem unzipping zips created by gulp-zip on Windows.

Spetnik commented 7 years ago

The suggestion from @timdp to add {nodir: true} worked for me as well. To prevent future complaints/"me toos" would you accept a PR to note this in the documentation?