clean up slim code using glob-all to make it more cross platform #227
Conversation
dschep
changed the title
|
hi @dschep - unfortunately getting an error now. With: It fails with error:
With just: It runs without an error, partially works because the zip is considerably smaller, but still does not remove "test" folders and files. Even with regex pattern matching like a described in #212. |
|
Thanks! Hmm. I'll have to look into the |
|
Hmm. Looked a bit more.. I have no clue why you're getting that find exec error. The tests all pass in circle, and I'd expect this one would fail the way you described: https://github.com/UnitedIncome/serverless-python-requirements/blob/3e0980605be8f0e72bc49792907868ed7f7ef410/test.bats#L111-L122 |
|
Wow, that's a really nice cleanup, thanks, it's cool to see and learn from refactoring |
lib/slim.js
Outdated
| let stripCmd = []; | ||
| const getStripCommand = (options, folderPath) => | ||
| process.platform !== 'win32' || isWsl || options.dockerizePip | ||
| ? ` && find ${folderPath} -name "*.so" -exec strip {} \\;` |
There was a problem hiding this comment.
I have noticed that again changing
` && find ${folderPath} -name "*.so" -exec strip {} \\; ` to
either ` && find ${folderPath} -name "*.so" -exec strip {} \;`
or ` && find ${folderPath} -name "*.so" -exec strip {} ;`
makes it run as expected on Win10.
There was a problem hiding this comment.
Interesting. pushing a commit with out the \\ to see how it fares in circle. It might have to be a platform dependent thing. I'd assume WSL should be treated like a *nix tho because it's running in as *nix shell.
There was a problem hiding this comment.
@dschep
Indeed, I was rather surprised too..
If it fails, could be interesting to also replace ; operator with +.
I did not have a good reason to use a semicolon, but it seems that + should also work and it does not generally need an escape: as mentioned here
There was a problem hiding this comment.
Good suggestion! Works great since strip supports getting multiple arguments.
lib/slim.js
Outdated
| stripCmd = stripCmd.concat(customPatterns); | ||
| const deleteFiles = (options, folderPath) => { | ||
| let patterns = [ | ||
| '**/*.so', |
There was a problem hiding this comment.
Also deleteing the .so files might not necessarily be safe as these are extension modules.
There was a problem hiding this comment.
DoH! Good catch. I didn't mean to delete those. that's what strip is for!
|
I think it should work now. Could you please test @sweepy84 and @dee-me-tree-or-love? Thanks! |
lib/slim.js
Outdated
| let stripCmd = []; | ||
| const getStripCommand = (options, folderPath) => | ||
| process.platform !== 'win32' || isWsl || options.dockerizePip | ||
| ? ` && find ${folderPath} -name "*.so" -exec strip {} +` |
There was a problem hiding this comment.
This again did not work on Win10 when I tried 😅
Serverless: Docker Image: lambci/lambda:build-python3.6
Error --------------------------------------------------
...
find: ‘strip’: No such file or directory
But now having changed it back to a semicolon with a different escape seems to work and supposedly should also be fine for *nix platforms... Could you try ` && find ${folderPath} -name "*.so" -exec strip {} ';' ` (yes, single quotes :D)
Sorry for making it so absurd to try different ways every time 😅
But it will call the strip command per each found .so file, whereas + should've processed as much files at once as possible (which might actually still be one per call)
There was a problem hiding this comment.
That's so odd. I'm fine with it being a strip call per .so. Just switched to ';'. Lets see if CI/*nix is happy. Windows should be happy per your experiments.
|
should work (for real this time)! |
|
Great! Worked well for me! |
|
It works!!! |
|
I've updated the readme too now 😃 |
Could you try this out @sweepy84? Install this branch with:
closes #212