Skip to content
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

Fixes Dockerless build fails to run in Linux #76 #77

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

marcellourbani
Copy link
Contributor

What did you implement:

Closes: #76

How did you verify your change:

Ran it on my own function which originated the bug

What (if anything) would need to be called out in the CHANGELOG for the next release:

Nothing

Short note:

  • code was trying to set attributes to the zip file to 755 rather than 0o755 aka 493 (unix permissions are useuallt expressed in octal)
  • file permissions in Admzip need to be shifted 16 bits. Not documented, but I found out in stackoverflow and is confirmed by admzip sources:
    image

@softprops
Copy link
Owner

Nice catch!

@softprops softprops merged commit f4203cb into softprops:master Jul 2, 2020
@softprops
Copy link
Owner

softprops commented Jul 2, 2020

I'm not sure why but after merging this I'm ci permission errors in ci when attempting
to run one of these lambdas https://github.com/softprops/serverless-rust/runs/829634826?check_suite_focus=true

@softprops
Copy link
Owner

I'll take a closer look tomorrow

@marcellourbani
Copy link
Contributor Author

Funny, it's precisely the issue this was supposed to fix!
And I'm pretty sure it's correct in an Unix-like environment, which should be the case even on windows for container based stuff.
I ran the integration tests on my box, they all pass except an unrelated one:
image

Afraid I can't help you further

@marcellourbani marcellourbani deleted the file_attributes_in_zip branch July 2, 2020 08:27
@softprops softprops mentioned this pull request Jul 3, 2020
@softprops
Copy link
Owner

@marcellourbani after going back and forth on this I think the issue you might have seen might have been related to a different factor. I did some more experimentation and eventual came to the conclusion that the 755 was the only value that worked in all three cases. If you are interested this repo builds lambdas locally on ubunutu latest, macox latest and windows latest then runs them using lambdaci docker containers as a smoke test. I added some debug logging to print the file permissions and in call cases the executable bits are set https://github.com/softprops/serverless-rust/runs/837468690?check_suite_focus=true

@marcellourbani
Copy link
Contributor Author

marcellourbani commented Jul 5, 2020

@softprops this is interesting!
Happy to read that it's passing the tests, but it's still buggy, the flags should be set to 0o755 or 0755, 755 is just wrong.

The code in my PR is correct, but fails because adm_zip has a bug

I opened a bug with them about this, works ok if the first byte is a 6 but not for a 7:

var AdmZip = require("adm-zip")
var zip = new AdmZip()
var content = "inner content of the file"
var buffer = Buffer.alloc(content.length, content)
zip.addFile("0o644.txt", buffer, "", 0o644 << 16)
zip.addFile("0o655.txt", buffer, "", 0o655 << 16)
zip.addFile("0o677.txt", buffer, "", 0o677 << 16)
zip.addFile("0o777.txt", buffer, "", 0o777 << 16)
zip.addFile("0o755.txt", buffer, "", 0o755 << 16)
zip.writeZip("./testattr.zip")
zipinfo testattr.zip                                                                    
Archive:  testattr.zip
Zip file size: 627 bytes, number of entries: 5
?rw-r--r--  1.0 fat       25 b- defN 20-Jul-05 11:01 0o644.txt
?rw-r-xr-x  1.0 fat       25 b- defN 20-Jul-05 11:01 0o655.txt
?rw-rwxrwx  1.0 fat       25 b- defN 20-Jul-05 11:01 0o677.txt
-rw----     1.0 fat       25 b- defN 20-Jul-05 11:01 0o755.txt
-rw----     1.0 fat       25 b- defN 20-Jul-05 11:01 0o777.txt
5 files, 125 bytes uncompressed, 135 bytes compressed:  -8.0%

Options:

  • fix adm_zip
  • replace with a working library like yazl
var yazl = require("yazl")
var fs = require("fs")
var zip2 = new yazl.ZipFile()
zip2.addBuffer(buffer, "0o644.txt", { mode: 0o100644 })
zip2.addBuffer(buffer, "0o655.txt", { mode: 0o100655 })
zip2.addBuffer(buffer, "0o677.txt", { mode: 0o100677 })
zip2.addBuffer(buffer, "0o777.txt", { mode: 0o100777 })
zip2.addBuffer(buffer, "0o755.txt", { mode: 0o100755 })
zip2.outputStream
  .pipe(fs.createWriteStream("testattr_yazl.zip"))
  .on("close", () => {
    exec("zipinfo ./testattr_yazl.zip", (e, o) => console.log(o))
  })
zip2.end()
zipinfo testattr_yazl.zip                                                                   
Archive:  testattr_yazl.zip
Zip file size: 627 bytes, number of entries: 5
-rw-r--r--  6.3 unx       25 b- defN 20-Jul-05 11:09 0o644.txt
-rw-r-xr-x  6.3 unx       25 b- defN 20-Jul-05 11:09 0o655.txt
-rw-rwxrwx  6.3 unx       25 b- defN 20-Jul-05 11:09 0o677.txt
-rwxrwxrwx  6.3 unx       25 b- defN 20-Jul-05 11:09 0o777.txt
-rwxr-xr-x  6.3 unx       25 b- defN 20-Jul-05 11:09 0o755.txt
5 files, 125 bytes uncompressed, 135 bytes compressed:  -8.0%

Tempted to send another PR to migrate to yazl if you're interested

@softprops
Copy link
Owner

code in my PR is correct, but fails because adm_zip has a bug

Ok.

Could you start with a failing test? The correct fix for me results in a deployable lambda. The current tests capture building on Linux and invoking in a lambda like environment.

I'd be more interested in learning where this is failing for you and how to reproduce that before attempting another change.

@softprops
Copy link
Owner

softprops commented Jul 5, 2020

Here is where I'm logging the permissions

ls -alh /tmp/ubuntu-latest-lambda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dockerless build fails to run in Linux
2 participants