fix: apply chmod at pkg creation time #3800
Conversation
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
apply chmod at package creation time instead of doing it when running the postinstall hook. Signed-off-by: Andres Correa Casablanca <[email protected]>
castarco
force-pushed
the
chmod-at-pkg-creation-time
branch
from
a27c8a0 to
fc19e89
Compare
| @@ -44,6 +44,7 @@ function generateNativePackage(platform, arch) { | |||
|
|
|||
| console.log(`Copy binary ${binaryTarget}`); | |||
| fs.copyFileSync(binarySource, binaryTarget); | |||
| fs.chmodSync(binaryTarget, 0o755); | |||
There was a problem hiding this comment.
The same way there is a try-catch block for the chmodSync call in the postinstall hook code, we could do the same here, because in theory it can fail the same way. However, that applies to every IO operation in this script (like copy), but that's not done here (I guess because it doesn't improve much on the potential error messages we could have, and because the pipeline would have crashed anyway).
There was a problem hiding this comment.
Nice. Didn't know that npm preserves execution permissions.
I wasn't able to find any documentation but this old issue confirms that it used to work (and should be fixed by now).
This may fix #3621
| @@ -25,17 +25,6 @@ if (binName) { | |||
| `The Rome CLI postinstall script failed to resolve the binary file "${binName}". Running Rome from the npm package will probably not work correctly.`, | |||
| ); | |||
| } | |||
|
|
|||
| if (binPath) { | |||
There was a problem hiding this comment.
Would it make sense to move the resolution failure messages to the rome start script instead and avoid using postinstall scripts altogether?
There was a problem hiding this comment.
The rome script already implements one out of the two error messages in the postinstall hook, so we could bring them in sync. But I still think we should keep that script, the call to chmod was only added there as a hack but its original purpose was to check that Rome is installed correctly and immediately alert the user if an issue was found, instead of having to wait for them to run the rome command.
| @@ -25,17 +25,6 @@ if (binName) { | |||
| `The Rome CLI postinstall script failed to resolve the binary file "${binName}". Running Rome from the npm package will probably not work correctly.`, | |||
| ); | |||
| } | |||
|
|
|||
| if (binPath) { | |||
There was a problem hiding this comment.
The rome script already implements one out of the two error messages in the postinstall hook, so we could bring them in sync. But I still think we should keep that script, the call to chmod was only added there as a hack but its original purpose was to check that Rome is installed correctly and immediately alert the user if an issue was found, instead of having to wait for them to run the rome command.
Summary
Fixes #3799 .
Test Plan
Testing this change is not trivial because it impacts CD pipelines. My suggestion:
.npmrcfile, by adding this line:ignore-scripts=truepostinstallhook has not been triggered because of our change on the.npmrcconfig file.As a side note, it would be great if some of the steps that are being executed in the CD pipeline were extracted into scripts that we could run locally. This way it would be easier to test some of these details. I'm not doing that in this PR because I believe it deserves some prior discussion, and I don't want to mix too many things in a single PR.
Signed-off-by: Andres Correa Casablanca [email protected]