Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix: apply chmod at pkg creation time #3800

Merged
merged 1 commit into from
Nov 21, 2022
Merged

fix: apply chmod at pkg creation time #3800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions npm/rome/scripts/generate-packages.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ function generateNativePackage(platform, arch) {

console.log(`Copy binary ${binaryTarget}`);
fs.copyFileSync(binarySource, binaryTarget);
fs.chmodSync(binaryTarget, 0o755);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

function updateWasmPackage(target) {
Expand Down
11 changes: 0 additions & 11 deletions npm/rome/scripts/postinstall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to move the resolution failure messages to the rome start script instead and avoid using postinstall scripts altogether?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try {
require("fs").chmodSync(binPath, 0o755);
} catch {
console.warn(
"The Rome CLI postinstall script failed to set execution permissions to the native binary. " +
"Running Rome from the npm package will probably not work correctly.",
);
}
}
} else {
console.warn(
"The Rome CLI package doesn't ship with prebuilt binaries for your platform yet. " +
Expand Down