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: $npm_execpath always points to npm, even when running npx #6762

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Sep 3, 2023

When running a script with npx (but not with npm exec), $npm_execpath is set to ".../npm/bin/npx-cli.js". This causes packages to fail to detect the package manager.

Now $npm_execpath will always be the path to npm-cli.js.

References

Fixes #6662

@rotu rotu marked this pull request as ready for review September 3, 2023 21:56
@rotu rotu requested a review from a team as a code owner September 3, 2023 21:56
@rotu rotu changed the title Fix incorrect $npm_execpath (fix) $npm_execpath always points to npm, even when running npx Sep 4, 2023
re-add flatOptions.npmBin
hoist nodeBin to config option and respect execPath override
@rotu rotu changed the title (fix) $npm_execpath always points to npm, even when running npx fix: $npm_execpath always points to npm, even when running npx Sep 5, 2023
@wraithgar
Copy link
Member

Not sure why CI is having the specific errors it is, but when I run config tests locally they don't pass, and there are coverage issues. Does npm test -w @npmcli/config pass locally for you?

@rotu
Copy link
Contributor Author

rotu commented Sep 5, 2023

I'm surprised it's trying to run against node <18. This commit definitely broke the runs on node 14: b8a5764

No, the CI is not currently passing for me locally. I suspect it has to do with this gotcha from the docs:

> When the entry point is not a CommonJS module, require.main is undefined, and the main module is out of reach.

I had to adjust some tests to account for moving logic out of the flatten helper.

I'll figure out the coverage issue once I verify the tests run as expected.

@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2023

The last commit (b8b1db3) I'm slightly unsure about.

This commit started writing to the variable: c3ba1da
But I can't figure out why we would ever read from this variable. It seems to maybe have originated in testing code here and accidentally copied into the main code flow: npm/config@fd6432e

EDIT: it also seems like inheriting this value is probably unexpected, given npm/config#12 for the closely related $INIT_CWD (which you may remember from the above-mentioned c3ba1da)

Your thoughts are much appreciated.

@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2023

BTW, as of now, test cases all pass for me and coverage is at 100%.

@wraithgar
Copy link
Member

wraithgar commented Sep 6, 2023

I agree that the process.env.NODE is in all likelihood a dependency injection leftover from a test. I don't know if that means we can remove it.

Historically we have been removing the ability to change things like this, for instance in npm 10 re removed the ci-name config. Previously we also did not allow folks to change what the path to npm was in scripts.

However, removing this could be a breaking change to folks. We're gonna have to leave it in. However I also think we shouldn't worry about covering that in testing. Can you add /* istanbul ignore next */ to the line before it, and put it back in?

I'll add a note to the npm 11 roadmap to remove it for real.

@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2023

However, removing this could be a breaking change to folks. We're gonna have to leave it in. However I also think we shouldn't worry about covering that in testing. Can you add /* istanbul ignore next */ to the line before it, and put it back in?

I thought about that and it makes me feel icky. The ignored path is the one MOST COMMON in practice. This PR is already a breaking change, and insofar as the NODE environment variable does anything, it’s a bug as well.

If you still insist, I can move it to another PR.

@wraithgar
Copy link
Member

Ok lemme noodle on it.

Ultimately the NODE environment is in all likelihood trying to solve the problem of having everything in side the npm lifecycle use the same node version as the one that npm was ran with. I do remember this being an issue in the past, we don't want to support running different versions of node or npm in subprocesses. In the past this manifested as refusing to support a config that told npm what version of node to run, that's an OS concern.

I think removing it may be the right choice here but as you can imagine we are trying to be extremely cautious with things that could break other folks.

@rotu
Copy link
Contributor Author

rotu commented Sep 8, 2023

Ok lemme noodle on it.

@wraithgar, what did your noodle say?

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

We can re-export NODE if an edge case we didn't know about comes to light.

@wraithgar wraithgar merged commit 7bf2374 into npm:latest Sep 8, 2023
32 of 38 checks passed
@github-actions github-actions bot mentioned this pull request Sep 8, 2023
@rotu
Copy link
Contributor Author

rotu commented Sep 8, 2023

Woohoo!

We can re-export NODE if an edge case we didn't know about comes to light.

I didn’t stop exporting $NODE to subprocess - I stopped npm from importing it from the parent process.

@wraithgar
Copy link
Member

Ok yeah that's what I get from typing from memory.

@rotu rotu deleted the dependent-peacock branch September 9, 2023 06:27
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.

[BUG] npm_execpath is set to the path of npx instead of npm
2 participants