Update npm install command to use --os flag #985
Conversation
🦋 Changeset detectedLatest commit: af863e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
||
| const additionalArgs = installOptions.additionalArgs ?? ""; | ||
| const installCommand = `npm install --platform=linux ${archOption} ${targetOption} ${libcOption} ${additionalArgs} ${installOptions.packages.join(" ")}`; | ||
| const installCommand = `npm install --os=linux ${archOption} ${targetOption} ${libcOption} ${additionalArgs} ${installOptions.packages.join(" ")}`; |
There was a problem hiding this comment.
We should provide both if it does not cause any issue, not just replace it
There was a problem hiding this comment.
@conico974 It looks like there is no --platform option. What about updating the code to use --os=${os.platform()}
It looks like it would help with local e2e on Mac
/cc @sommeeeer
There was a problem hiding this comment.
See https://sharp.pixelplumbing.com/install/#npm-v10 (kudos to Magnus for finding that link)
There was a problem hiding this comment.
We could introduce another optional parameter to InstallOptions that is os and defaults to linux. Then here we set it to what you suggested if the config.imageOptimization.loader is fs-dev. WDYT?
Update: However, does glibc work on macOS?
There was a problem hiding this comment.
+1 to your idea @sommeeeer
We could also detect the CPU (i.e. arm64 vs x64)
Update: However, does glibc work on macOS?
I'll test and report.
There was a problem hiding this comment.
I don't remember when we added the --platform options TBH, it's a very old one.
I'm fine with adding the os option.
Autodetecting is something that we need to be able to override though, this could break some deployment otherwise.
There was a problem hiding this comment.
Hey, been busy with other things and haven't gotten round to this PR. But if I understand correctly, autodetecting would break my deployment since I usually deploy through GitHub actions but on rare occasions I've had to deploy from local to fix issues.
There was a problem hiding this comment.
What they suggest is only if you use the fs-dev image loader (which is likely not your case).
But even in this case I think that if the user provide some options we should respect it
commit: |
There was a problem hiding this comment.
Thanks for the PR and updates @niklaswallerstedt.
I have rebased the PR and squashed the commits.
I'll go ahead and merge this fix (--platform seems to never have existed but should be --os).
We'll address the follow up discussed in the comments in #971 as it is where those are needed.
|
The pre-releasing step fails... merging anyway |
Resolves a problem where sharp after 0.36.2 would not install correctly.
--osinstead of platform- updatessharpto latest version