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: force, buildFromSource cli opts are ignored in yarn #140

Merged
merged 1 commit into from
Apr 4, 2021
Merged

Fix: force, buildFromSource cli opts are ignored in yarn #140

merged 1 commit into from
Apr 4, 2021

Conversation

joaomoreno
Copy link
Contributor

@joaomoreno joaomoreno commented Jan 28, 2021

The issue comes from this changes 4 years ago which fixed the issue that sources were always compiling when used in yarn: e9065dd

This fix seems irrelevant by now, when one looks at the code paths. It also blocks the usage of the force and buildFromSource options. It seems safe to simply remove the check altogether. I have verified that doing so makes prebuild-install correctly download or build from source when explicitly told so, using yarn.

Co-authored-by: Benjamin Pasero benjpas@microsoft.com
Fixes: #139

this removes an old check for whether yarn is executing the script which breaks the force and buildFromSource cli options. the check seems irrelevant by now so it seems safe to remove

Co-authored-by: Benjamin Pasero <benjpas@microsoft.com>
@vweevers
Copy link
Member

This fix seems irrelevant by now, when one looks at the code paths.

Can you elaborate? Is it b6f3b36 that made the fix irrelevant?

@joaomoreno
Copy link
Contributor Author

Good question. And now that I look at it again, it just seems that e9065dd was simply a bad fix, since it just makes it that in yarn, it would always load from https, regardless of the buildFromSource opt value.

What would be interesting to find out was: what was making prebuild-install misbehave in the presence of yarn, such that that commit was necessary?

@vweevers
Copy link
Member

vweevers commented Apr 4, 2021

What would be interesting to find out was: what was making prebuild-install misbehave in the presence of yarn, such that that commit was necessary?

prebuild-install/bin.js

Lines 45 to 49 in e9065dd

if (util.isYarnPath(execPath) && /node_modules/.test(process.cwd())) {
// From yarn repository
} else if (!(typeof pkg._from === 'string')) {
log.info('install', 'installing inside prebuild-install directory, skipping download.')
process.exit(1)

The first if-branch was added because yarn would always match the second if-branch, because it didn't add metadata (like _from) to package.json files on disk, like npm did (until npm 7).

@vweevers
Copy link
Member

vweevers commented Apr 4, 2021

It seems safe to simply remove the check altogether.

I agree, as prebuild-install has been updated to be compatible with npm 7 which (long story short) means that the absence of pkg._from is no longer an issue.

@vweevers vweevers added the semver-patch Bug fixes that are backward compatible label Apr 4, 2021
@vweevers vweevers merged commit 8cb1ced into prebuild:master Apr 4, 2021
@vweevers
Copy link
Member

vweevers commented Apr 4, 2021

6.1.1

@joaomoreno joaomoreno deleted the remove-yarn-check branch April 6, 2021 08:13
@joaomoreno
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Bug fixes that are backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI options (force, buildFromSource) are ignored when using yarn
2 participants