Adjust install script for m1 macs & switch to Node 16 #10291
Conversation
pgrzesik
force-pushed
the
adjust-install-script-for-m1-macs
branch
from
fa976ff
to
dcc0a86
Compare
Codecov Report
@@ Coverage Diff @@
## master #10291 +/- ##
==========================================
- Coverage 85.40% 85.38% -0.02%
==========================================
Files 339 339
Lines 13877 13880 +3
==========================================
Hits 11852 11852
- Misses 2025 2028 +3
Continue to review full report at Codecov.
|
|
After review, I will change the required checks so the PR doesn't have any failing |
scripts/pkg/build.js
Outdated
| @@ -20,7 +20,7 @@ const spawnOptions = { cwd: serverlessPath, stdio: 'inherit' }; | |||
| // It's due to fact that npm tends to issue buggy releases | |||
| // Node.js confirms on given version before including it within its bundle | |||
| // Version mappings reference: https://nodejs.org/en/download/releases/ | |||
| await spawn('npm', ['install', '--no-save', 'npm@6.14.15'], spawnOptions); | |||
| await spawn('npm', ['install', '--no-save', 'npm@8.1.0'], spawnOptions); | |||
There was a problem hiding this comment.
Just this change I'd save for v3. As it'll upgrade npm to version which will now also install serverless in node_modules if plugin has it set as peer dependency. I think this is ok, but it may come as unexpected shift to some users, so it's better if it comes with new major
There was a problem hiding this comment.
Makes sense, I've reverted from that change
| @@ -30,6 +30,9 @@ fi | |||
| MACHINE_TYPE=`uname -m` | |||
| if [[ $MACHINE_TYPE == "x86_64" ]]; then | |||
| ARCH='x64' | |||
| elif [[ $OSTYPE == "darwin"* ]] && [[ $MACHINE_TYPE == "arm64" ]]; then | |||
There was a problem hiding this comment.
Can you confirm wether we need to adjust this util:
serverless/lib/utils/standalone.js
Lines 15 to 25 in d52526b
I assume that not, as I take that binary as run through rosetta will internally assume we run things on x84 architecture, but if it'll detect it's ARM, then it'll generate wrong link for binary update
There was a problem hiding this comment.
Great catch, I've tested and actually, it reports arm64 - I've adjusted and tested that update works correctly on M1 macs too
pgrzesik
force-pushed
the
adjust-install-script-for-m1-macs
branch
from
dcc0a86
to
157f1b3
Compare
lib/utils/standalone.js
Outdated
| @@ -13,6 +13,9 @@ const platform = (() => { | |||
| } | |||
| })(); | |||
| const arch = (() => { | |||
| if (process.arch === 'arm64' && process.platform === 'darwin') { | |||
There was a problem hiding this comment.
It'll be cleaner to include that logic in below switch statement
There was a problem hiding this comment.
Changed, I wanted to highlight that edge case and handle it first, but you're right that it could've been done in a cleaner way
pgrzesik
force-pushed
the
adjust-install-script-for-m1-macs
branch
from
157f1b3
to
68581da
Compare
|
Github issue has been resolved, I'm merging this and updating the required checks for |
Scope: