Fixes Dockerless build fails to run in Linux #76

marcellourbani commented 4 years ago

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:

softprops commented 4 years ago

Nice catch!

softprops commented 4 years ago

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 commented 4 years ago

I'll take a closer look tomorrow

marcellourbani commented 4 years ago

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:

Afraid I can't help you further

softprops commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

Here is where I'm logging the permissions https://github.com/softprops/serverless-rust/blob/0e818ddd70319ca213c5e7acc64c479171295972/.github/workflows/main.yml#L88

softprops / serverless-rust