Revert "build,windows: implement PEP514 python detection"

[seishun]

This reverts commit 614dbbd.

Fixes: #16864

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

build

[bnoordhuis]

Can you add more detail to the commit log?

[seishun]

@bnoordhuis done, PTAL.

[seishun]

CI: https://ci.nodejs.org/job/node-test-pull-request/11700/

[bnoordhuis]

Thanks, LGTM.

[seishun]

Requesting fast-track since this is a revert.

[bnoordhuis]

Go for it.

[seishun]

@evanlucas do you approve just the pull request, or also the fast-tracking request?

The pull request can be landed once 2 or more collaborators approve both the pull request and the fast-tracking request, and the necessary CI testing is done.

[evanlucas]

@seishun both, sorry, I should have clarified. +1 to fast tracking

[seishun]

Thanks, going to land in 24 hours if there are no objections.

[refack]

I think the motivation is low nither for a revert of a commit that landed 5 months ago, nor for fast-track.

[refack]

I think the motivation is low nither for a revert of a commit that landed 5 months ago, nor for fast-track.

[refack]

That sacrifices clarity to support a rare use case and causes surprising behavior when Python is not found.

The "rare use case" has been reported by many first time users (nodejs/CTC#147 (comment)), and has not been reported since.

P.S. this PR does not CC the relevant parties @nodejs/build @nodejs/platform-windows

[seishun]

The report you linked looks like hearsay. It might as well have been a misunderstanding. Either way:

In particular, BUILDING.md on Windows does not elaborate on adding Python to the path file..

That should be fixed. Running tests already requires tools from Git for Windows to be in PATH, so requiring Python as well seems reasonable.

[refack]

#17298 has a proposed fix that improves the error message without losing PEP514 detection

[benjamingr]

That should be fixed. Running tests already requires tools from Git for Windows to be in PATH, so requiring Python as well seems reasonable.

For what it's worth, not a single first timer had issues with git in path (since they typically used git bash to build anyway) - but a few had issues with Python.

I'm not expressing an opinion about whether or not Node.js should help users in such cases - it was clear though.

[seishun]

since they typically used git bash to build anyway

vcbuild doesn't work in Git Bash though.

[gibfahn]

Requesting fast-track since this is a revert.

I don't think revert automatically means fast-track.

Fixes #16864

Isn't that just a case of an unhelpful error message? Python is required to actually build and test node right?

That sacrifices clarity to support a rare use case and causes surprising behavior when Python is not found.

We landed changes to configure to do similar things to help users confused by the python2 python3 thing, this doesn't seem that different.

node/configure

Lines 3 to 12 in 9de15de

	# Locate python2 interpreter and re-execute the script. Note that the
	# mix of single and double quotes is intentional, as is the fact that
	# the ] goes on a new line.
	_=[ 'exec' '/bin/sh' '-c' '''
	which python2.7 >/dev/null && exec python2.7 "$0" "$@"
	which python2 >/dev/null && exec python2 "$0" "$@"
	exec python "$0" "$@"
	''' "$0" "$@"
	]
	del _

I think trying to help users out on Windows is a good thing to do, so I'd rather we fix the issue (e.g. with #17298) than just revert.

The report you linked looks like hearsay. It might as well have been a misunderstanding. Either way:

Are you saying you think there is no scenario in which this change helps people?

[seishun]

I don't think revert automatically means fast-track.

No, but it means I can request it.

We landed changes to configure to do similar things to help users confused by the python2 python3 thing, this doesn't seem that different.
Are you saying you think there is no scenario in which this change helps people?

I think we're talking about different things. I'm all for helpful error messages, but going out of our way to locate Python when it's not in PATH seems excessive.

[gibfahn]

Requesting fast-track since this is a revert.
No, but it means I can request it.

Okay, I guess I misunderstood what you meant by since (I hadn't seen the latest update to the Collaborator Guide).

I think we're talking about different things. I'm all for helpful error messages, but going out of our way to locate Python when it's not in PATH seems excessive.

If it can be made to work, and it makes life easier for users, and it's not too difficult to maintain, I don't see the harm in it. In my experience people who install things through GUIs (e.g. me before I started using Linux 😁 ) rarely have experience manually modifying their paths, so why not help them?

[seishun]

If it can be made to work, and it makes life easier for users, and it's not too difficult to maintain, I don't see the harm in it.

It does add a maintenance burden, at least in the current form: #17298 (comment)

rarely have experience manually modifying their paths, so why not help them?

The Python installer has an option to add it to PATH. I suppose we could mention that.

[gibfahn]

If it can be made to work, and it makes life easier for users, and it's not too difficult to maintain

It does add a maintenance burden, at least in the current form: #17298 (comment)

I'm hoping @refack can get #17298 to a point where everyone is happy with it.

The Python installer has an option to add it to PATH. I suppose we could mention that.

Probably, although I think #17046 should make life much easier for people who don't already have it installed. This seems more for people who already have it, but didn't tick that box.

[seishun]

This seems more for people who already have it, but didn't tick that box.

As in, people who have already been using Python for some time, but somehow never added it to PATH? That seems an unlikely combination.

[benjamingr]

As in, people who have already been using Python for some time, but somehow never added it to PATH? That seems an unlikely combination.

Again - this is something that's been happening in practice. I'm fine with mentioning it in the guide, having an FAQ, or taking a different approach than @refack's fix.

I'm less fine with fast-tracking non-breaking changes in development setup without giving the collaborator who landed them chance to respond. It's not your fault since you followed protocol - we should probably PR the collaborator guide and amend that.

[refack]

The root cause is that the GUI installer does not add python to PATH by default (because of the adoption of PEP514):

So we should keep changes around this in coordination with #17046 (a silent install will probably not add python to PATH)

[seishun]

Git for Windows doesn't add Unix tools required for vcbuild test to PATH by default either:

The Boxstarter script in #17046 uses the /GitAndUnixToolsOnPath parameter, perhaps there's something similar for Python?

[bzoz]

Boxstarter python is added to the PATH

[seishun]

@nodejs/collaborators we need to decide whether to merge this PR or #17804. The reasons why I prefer this one are mentioned in #17804. This PR has 3 approvals and 1 change request (not sure if it's still relevant? cc @refack), the other one has 2 approvals, so the opinions seem divided.

[BridgeAR]

I am closing this as it seems like a decision is needed. This was blocked by @refack and there is a alternative without real complaints. @seishun if you feel differently, please reopen.

[seishun]

I would prefer to keep this open until either #17804 or #18621 lands.

[BridgeAR]

I am going to close this since a mentioned alternative has landed. @seishun if this is not correct, please reopen.