Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix the preinstall function logic #55

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aminya
Copy link

@aminya aminya commented Nov 18, 2022

It simplifies the preinstall logic to call the load function and report errors in case of not finding a prebuild. It then falls back to the build.

Fixes #54

Test result:

image

@vweevers
Copy link
Member

This is not the right place to implement this. But before we dive into that, let's first discuss if we even want this, because I'm not sure.

On the one hand, per the unix philosophy, output should not include unnecessary information. That a build was not found is unnecessary information because there's a fallback to compiling from source. If that succeeds (and we expect it to) then output should be empty. Also because "no native build was found" can be misinterpreted as an error (#42). If it doesn't succeed, then npm i --verbose can be used to opt-in to that extra information.

On the other hand, npm is swallowing the output of install scripts nowadays, so we already get the desired behavior of "silent on success". So we could decide to always be verbose.

@aminya
Copy link
Author

aminya commented Nov 18, 2022

But before we dive into that, let's first discuss if we even want this, because I'm not sure.

This helps the developers to understand the reasons why their package installation goes through the build process instead of just loading the file. I'll open an issue for npm if npm doesn't report the error logs by default.

On the one hand, per the unix philosophy, output should not include unnecessary information

As per my government's rules, error logging must be enabled on all Information Technology (IT) assets throughout the enterprise. So, not sure if the philosophical opinions come into play here.

Also, the previous code was executing an unvalidated process.argv[2]. It can be exploited as remote code execution. I haven't spent much time looking into the code, but I think it is a CVE target. I have sent an email to the author.

@vweevers
Copy link
Member

vweevers commented Nov 18, 2022

As per my government's rules, error logging must be enabled

Interesting, but the point is, it's not an error per se.

Also, the previous code was executing an unvalidated process.argv[2]. I am not sure if it can be exploited as remote code execution. I haven't spent much time looking into the code, but it might be a CVE target.

No, because it's sourced from an npm script. I.e. there's no difference in risk between node-gyp-build dangerous-command and node-gyp-build && dangerous-command. But, if I'm wrong and there is a real vulnerability, then please contact one of us privately with details.

@vweevers
Copy link
Member

I'll open an issue for npm if npm doesn't report the error logs by default.

It swallows the output of an install script unless that script exited with a non-zero code. Which I think fits your requirements.

@aminya
Copy link
Author

aminya commented Nov 18, 2022

Yeah, so when the build also fails, I can know why the to prebuild was not picked up in the first place.

bin.js Outdated
Comment on lines 30 to 42
if (code || !process.argv[3]) process.exit(code)
exec(process.argv[3]).on('exit', function (code) {
process.exit(code)
})
if (code) process.exit(code)
})
}

function preinstall () {
if (!process.argv[2]) return build()
exec(process.argv[2]).on('exit', function (code) {
if (code) process.exit(code)

Choose a reason for hiding this comment

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

process.argv[2] and process.argv[3] are used here to allow the user to specify commands that should be run before and after the build, respectively. It looks like this PR would currently remove that functionality.

load()
} catch (err) {
// report the error and fall to a build
console.error(err.message)
Copy link

Choose a reason for hiding this comment

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

I don't think this should be an error. That would imply the consuming package has done something wrong or has an unexpected condition to recover from.

More likely in my experience, the package was simply not built.

@rotu
Copy link

rotu commented Jun 14, 2024

The intent of node-gyp-build seems to be to be to make loading native bindings a smooth experience. Adding a message here doesn't really do that and it gets buried under the already-confusing log messages of node-gyp that follow.

I would definitely be for making the situation less confusing. Maybe better guidance for --build-from-source/--fallback-to-build? Or if your package does have to be built on install instead of on demand, use something like https://www.npmjs.com/package/bindings ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report more information when a prebuild is not found
4 participants