Accept Python 3 by default - #1844

Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom
joaocgreis:joaocgreis-J7N-python3-3
Closed

Accept Python 3 by default#1844
joaocgreis wants to merge 1 commit into
nodejs:masterfrom
joaocgreis:joaocgreis-J7N-python3-3

Conversation

@joaocgreis

@joaocgreisjoaocgreis commented Jul 23, 2019•
edited
Loading

Copy link
Copy Markdown
Member
Checklist
Description of change

This depends on #1843, I will rebase when that PR lands. Only a971947 needs to be reviewed here.

This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.

@nodejs/node-gyp please not that we cover little more than a basic hello world in our tests, there are probably parts of Gyp that are not covered and are not compatible with Python 3. We will have to be extra careful shipping the node-gyp version that includes this.

@joaocgreis

joaocgreis commented Jul 23, 2019

Copy link
Copy Markdown
MemberAuthor

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/152/

richardlau
richardlau reviewed Jul 23, 2019
View reviewed changes
Comment threadlib/find-python.js
cclauss
cclauss approved these changes Jul 23, 2019
View reviewed changes

@cclausscclauss left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome work here!

Can you please add a Python 3 on macOS on Travis CI test like #1846 just to make sure that is passing?

@joaocgreis

joaocgreis commented Jul 24, 2019

Copy link
Copy Markdown
MemberAuthor

@cclauss I think #1846 is the proper place to do it, this PR might take a while to land. I'll rebase on top of that PR and adjust accordingly when it lands!

@joaocgreis

joaocgreis commented Jul 24, 2019

Copy link
Copy Markdown
MemberAuthor

This is blocked by #1846, we can't land this with OSX failing.

@cclausscclauss mentioned this pull request Jul 24, 2019
Closed
4 tasks
@rvagg

rvagg commented Jul 26, 2019

Copy link
Copy Markdown
Member

seems good to me but I'm confused on whether the macos problems block this and #1843 or if they can land independently of that?

@cclauss

cclauss commented Jul 26, 2019

Copy link
Copy Markdown
Contributor

We can land this with the macOS tests still running in allow_failures mode. Our README clearly states that Python 3 is not supported which aligns with Travis CI's intent for allow_failures

[allow_failures] lets you add in experimental and preparatory builds to test against versions or configurations that you are not ready to officially support.

@joaocgreis

joaocgreis commented Jul 26, 2019

Copy link
Copy Markdown
MemberAuthor

#1843 has landed. In there, node-gyp only uses Python 3 if EXPERIMENTAL_NODE_GYP_PYTHON3 is defined, so it should not break anything we currently have.

This PR removes that flag, making node-gyp accept Python 3. So, if a release is shipped with this PR, node-gyp will start using Python 3 in the users computers, and any issue we have will manifest there.

Currently, we know Python 3 is failing on osx, so shipping a release with this would break node-gyp on every osx machine that has Python 3 installed.

We know that our tests pass with Python 3 on Windows and Linux, but our tests don't have a good coverage of gyp. It's likely that there are modules out there that use parts of gyp that we don't test here, and those would be broken as well. GYP3 could help a lot with this.

So, we can't land this PR until all failures that we know of are fixed.

joaocgreis added a commit to JaneaSystems/node-gyp that referenced this pull request Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9be02bc
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed the joaocgreis-J7N-python3-3 branch 2 times, most recently from 9be02bc to 9318e21CompareJuly 26, 2019 15:19
joaocgreis added a commit to joaocgreis/node-gyp that referenced this pull request Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9318e21
PR-URL: nodejs#1844
joaocgreis added a commit to JaneaSystems/node-gyp that referenced this pull request Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
b154b07
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed the joaocgreis-J7N-python3-3 branch from 9318e21 to b154b07CompareJuly 26, 2019 15:27
@sam-githubsam-github mentioned this pull request Sep 6, 2019
Closed
@sam-github

sam-github commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

nodejs/node#29246 bumped into this.

I'm not sure what the plan is. If I'm missing the conversation elsewhere, pls point me there! I don't want to fragment an existing conversation.

node's tests can't pass on a py3-only machine until npm has a copy of node-gyp that supports py3.

Does npm even get node-gyp from this repo? :-( Sorry, you can see how late to this "party" I'm arriving.

@bnoordhuis

bnoordhuis commented Sep 6, 2019

Copy link
Copy Markdown
Member

Does npm even get node-gyp from this repo?

Yes. :-)

@joaocgreis

joaocgreis commented Sep 10, 2019

Copy link
Copy Markdown
MemberAuthor

@sam-github my comment above is still a valid summary of the current situation. Going forward, I believe the options on the table are either to use GYP3 (#1845) or fix the osx issues (@cclauss might be working on that).

This PR is ready to be rebased and land landed when we believe Python 3 support to be ready. However, this PR does not change any Python code, it only removes the block to run on Python 3.

@cclausscclauss mentioned this pull request Sep 11, 2019
Closed
@madacolmadacol mentioned this pull request Sep 12, 2019
Closed
This was referenced Sep 23, 2019
Closed
Closed
Closed
joaocgreis added a commit to joaocgreis/node-gyp that referenced this pull request Sep 25, 2019
@joaocgreis
lib: accept Python 3 by default
f7a28c3
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed the joaocgreis-J7N-python3-3 branch from b154b07 to f7a28c3CompareSeptember 25, 2019 13:39
@joaocgreis

joaocgreis commented Sep 25, 2019

Copy link
Copy Markdown
MemberAuthor

Rebased to make Travis run again, but it's still red. Current status: #1844 (comment) and #1844 (comment)

@cclauss

cclauss commented Sep 25, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

The macOS error looks related to #1854 so I will attack it...

Fixed in Decode stdout on Python 3#1890

cclauss added a commit to cclauss/node-gyp that referenced this pull request Sep 25, 2019
@cclauss
Testnodejs#1844andnodejs#1854
9f89138
This was referenced Sep 25, 2019
Closed
Closed
@cclauss

cclauss commented Sep 26, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Please rebase again to resolve conflict and ensure macOS now passes.

@cclauss

cclauss commented Sep 26, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?

  • On a machine that has both Python 2 and Python 3 installed?
  • On a machine that only has Python 3 installed?

joaocgreis added a commit to JaneaSystems/node-gyp that referenced this pull request Sep 26, 2019
@joaocgreis
lib: accept Python 3 by default
a490ca5
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed the joaocgreis-J7N-python3-3 branch from f7a28c3 to a490ca5CompareSeptember 26, 2019 13:09
joaocgreis added a commit to joaocgreis/node-gyp that referenced this pull request Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
1276037
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed the joaocgreis-J7N-python3-3 branch from a490ca5 to 1276037CompareSeptember 30, 2019 05:28
joaocgreis added a commit to joaocgreis/node-gyp that referenced this pull request Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3066b03
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed the joaocgreis-J7N-python3-3 branch from 1276037 to 3066b03CompareSeptember 30, 2019 05:47
joaocgreis added a commit to joaocgreis/node-gyp that referenced this pull request Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
167b563
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed the joaocgreis-J7N-python3-3 branch 2 times, most recently from 0ffb1aa to 167b563CompareSeptember 30, 2019 06:43
@joaocgreis
joaocgreisforce-pushed the joaocgreis-J7N-python3-3 branch from 167b563 to 05138c8CompareSeptember 30, 2019 06:47
@joaocgreis

joaocgreis commented Sep 30, 2019

Copy link
Copy Markdown
MemberAuthor

Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?

Doesn't "have" to, but I'd rather keep it in most (if not all) of the entries in Travis, since not having it was the reason we didn't know what Python was actually being run before. This is a good way to avoid surprises.

On a machine that has both Python 2 and Python 3 installed?
On a machine that only has Python 3 installed?

Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2. Whatever exists first is what's used.

joaocgreis added a commit that referenced this pull request Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3d1c60a
PR-URL: #1844
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis

joaocgreis commented Sep 30, 2019

Copy link
Copy Markdown
MemberAuthor

Hit an issue because of isaacs/minizlib#9 - Travis kept 1.3.1 in cache. Should be fixed now, forced update in one run and Travis updated the cache.

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/155/ ✔️

Landed in 3d1c60a

@sam-github

sam-github commented Oct 2, 2019

Copy link
Copy Markdown
Contributor

Does this change which version of python is chosen? I can't tell from the diff.

I.e

  • with python2, python3, and python (2.7) in /usr/bin, which will get chosen?
  • with python2, python3, and python (3.6) in /usr/bin, which will get chosen?

I don't have any concerns either way, but I think we should call out in the docs/changelog if the order of choice changed, right now it just says Python 3 is accepted, not under which conditions is preferred, if ever.

@cclauss

cclauss commented Oct 2, 2019

Copy link
Copy Markdown
Contributor

Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2.
Whatever exists first is what's used.

I take that to mean python will be used.

@sam-github

sam-github commented Oct 3, 2019

Copy link
Copy Markdown
Contributor

This makes node-gyp accept Python 3 and look for it before looking for Python 2, making Python 3 the default.

@joaocgreis Can I edit this description? I think that isn't quite accurate, it should be

This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.

Personally, I suggest that this PR can and should land on 5.x, with the minor tweek of moving python3 to afterpython2, so that users will Python 2 if they have it.

cf #1892 (comment) and previous discussion

@sam-githubsam-github mentioned this pull request Oct 3, 2019
Closed
4 tasks
rvagg pushed a commit that referenced this pull request Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
4215054
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg pushed a commit that referenced this pull request Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
07f81f1
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@neilshneilsh mentioned this pull request Oct 28, 2019
Merged
@rvaggrvagg mentioned this pull request Nov 18, 2019
Closed
@rvaggrvagg mentioned this pull request Feb 3, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@cclausscclausscclauss approved these changes

Assignees

No one assigned

Labels

Pythonsemver-major

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joaocgreis@rvagg@cclauss@sam-github@bnoordhuis@richardlau
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Accept Python 3 by default - #1844
Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Closed
Accept Python 3 by default#1844
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Conversation
@joaocgreis
@joaocgreisjoaocgreis
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Checklist
Description of change
This depends on #1843, I will rebase when that PR lands. Only a971947 needs to be reviewed here.
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
@nodejs/node-gyp please not that we cover little more than a basic hello world in our tests, there are probably parts of Gyp that are not covered and are not compatible with Python 3. We will have to be extra careful shipping the node-gyp version that includes this.
@joaocgreis
joaocgreis
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/152/
richardlau
richardlau
reviewed
Jul 23, 2019
View reviewed changes
Comment threadlib/find-python.js
cclauss
cclauss
approved these changes
Jul 23, 2019
View reviewed changes
@cclausscclauss
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Awesome work here!
Can you please add a Python 3 on macOS on Travis CI test like #1846 just to make sure that is passing?
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
@cclauss I think #1846 is the proper place to do it, this PR might take a while to land. I'll rebase on top of that PR and adjust accordingly when it lands!
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
This is blocked by #1846, we can't land this with OSX failing.
@cclausscclauss
mentioned this pull request
Jul 24, 2019
 Closed

4 tasks
@rvagg
rvagg
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
seems good to me but I'm confused on whether the macos problems block this and #1843 or if they can land independently of that?
@cclauss
cclauss
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
We can land this with the macOS tests still running in allow_failures mode. Our README clearly states that Python 3 is not supported which aligns with Travis CI's intent for allow_failures
[allow_failures] lets you add in experimental and preparatory builds to test against versions or configurations that you are not ready to officially support.
@joaocgreis
joaocgreis
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
#1843 has landed. In there, node-gyp only uses Python 3 if EXPERIMENTAL_NODE_GYP_PYTHON3 is defined, so it should not break anything we currently have.
This PR removes that flag, making node-gyp accept Python 3. So, if a release is shipped with this PR, node-gyp will start using Python 3 in the users computers, and any issue we have will manifest there.
Currently, we know Python 3 is failing on osx, so shipping a release with this would break node-gyp on every osx machine that has Python 3 installed.
We know that our tests pass with Python 3 on Windows and Linux, but our tests don't have a good coverage of gyp. It's likely that there are modules out there that use parts of gyp that we don't test here, and those would be broken as well. GYP3 could help a lot with this.
So, we can't land this PR until all failures that we know of are fixed.
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9be02bc
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
9be02bc to
9318e21CompareJuly 26, 2019 15:19
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9318e21
PR-URL: nodejs#1844
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
b154b07
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
9318e21 to
b154b07CompareJuly 26, 2019 15:27
@sam-githubsam-github
mentioned this pull request
Sep 6, 2019

Closed

@sam-github
sam-github
commented
Sep 6, 2019

Copy link


Copy Markdown

Contributor
nodejs/node#29246 bumped into this.
I'm not sure what the plan is. If I'm missing the conversation elsewhere, pls point me there! I don't want to fragment an existing conversation.
node's tests can't pass on a py3-only machine until npm has a copy of node-gyp that supports py3.
Does npm even get node-gyp from this repo? :-( Sorry, you can see how late to this "party" I'm arriving.
@bnoordhuis
bnoordhuis
commented
Sep 6, 2019

Copy link


Copy Markdown

Member
Does npm even get node-gyp from this repo?
Yes. :-)
@joaocgreis
joaocgreis
commented
Sep 10, 2019

Copy link


Copy Markdown

MemberAuthor
@sam-github my comment above is still a valid summary of the current situation. Going forward, I believe the options on the table are either to use GYP3 (#1845) or fix the osx issues (@cclauss might be working on that).
This PR is ready to be rebased and land landed when we believe Python 3 support to be ready. However, this PR does not change any Python code, it only removes the block to run on Python 3.
@cclausscclauss
mentioned this pull request
Sep 11, 2019

Closed

@madacolmadacol
mentioned this pull request
Sep 12, 2019

Closed


This was referenced Sep 23, 2019

Closed

 Closed


Closed

joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 25, 2019
@joaocgreis
lib: accept Python 3 by default
f7a28c3
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
b154b07 to
f7a28c3CompareSeptember 25, 2019 13:39
@joaocgreis
joaocgreis
commented
Sep 25, 2019

Copy link


Copy Markdown

MemberAuthor
Rebased to make Travis run again, but it's still red. Current status: #1844 (comment) and #1844 (comment)
@cclauss
cclauss
commented
Sep 25, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The macOS error looks related to #1854 so I will attack it...
Fixed in Decode stdout on Python 3#1890
cclauss added a commit
to cclauss/node-gyp
that referenced
this pull request
Sep 25, 2019
@cclauss
Testnodejs#1844andnodejs#1854
9f89138

This was referenced Sep 25, 2019
 Closed

 Closed

@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Please rebase again to resolve conflict and ensure macOS now passes.
@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
  • On a machine that has both Python 2 and Python 3 installed?
  • On a machine that only has Python 3 installed?
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Sep 26, 2019
@joaocgreis
lib: accept Python 3 by default
a490ca5
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
f7a28c3 to
a490ca5CompareSeptember 26, 2019 13:09
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
1276037
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
a490ca5 to
1276037CompareSeptember 30, 2019 05:28
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3066b03
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
1276037 to
3066b03CompareSeptember 30, 2019 05:47
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
167b563
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
0ffb1aa to
167b563CompareSeptember 30, 2019 06:43
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
167b563 to
05138c8CompareSeptember 30, 2019 06:47
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
Doesn't "have" to, but I'd rather keep it in most (if not all) of the entries in Travis, since not having it was the reason we didn't know what Python was actually being run before. This is a good way to avoid surprises.
On a machine that has both Python 2 and Python 3 installed?

On a machine that only has Python 3 installed?
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2. Whatever exists first is what's used.
joaocgreis added a commit
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3d1c60a
PR-URL: #1844
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Hit an issue because of isaacs/minizlib#9 - Travis kept 1.3.1 in cache. Should be fixed now, forced update in one run and Travis updated the cache.
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/155/ ✔️
Landed in 3d1c60a
@sam-github
sam-github
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Does this change which version of python is chosen? I can't tell from the diff.
I.e
  • with python2, python3, and python (2.7) in /usr/bin, which will get chosen?
  • with python2, python3, and python (3.6) in /usr/bin, which will get chosen?
I don't have any concerns either way, but I think we should call out in the docs/changelog if the order of choice changed, right now it just says Python 3 is accepted, not under which conditions is preferred, if ever.
@cclauss
cclauss
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2.

Whatever exists first is what's used.
I take that to mean python will be used.
@sam-github
sam-github
commented
Oct 3, 2019

Copy link


Copy Markdown

Contributor
This makes node-gyp accept Python 3 and look for it before looking for Python 2, making Python 3 the default.
@joaocgreis Can I edit this description? I think that isn't quite accurate, it should be
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
Personally, I suggest that this PR can and should land on 5.x, with the minor tweek of moving python3 to afterpython2, so that users will Python 2 if they have it.
cf #1892 (comment) and previous discussion
@sam-githubsam-github
mentioned this pull request
Oct 3, 2019
 Closed

4 tasks
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
4215054
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
07f81f1
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@neilshneilsh
mentioned this pull request
Oct 28, 2019
 Merged

@rvaggrvagg
mentioned this pull request
Nov 18, 2019
 Closed

@rvaggrvagg
mentioned this pull request
Feb 3, 2020
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@cclausscclausscclauss approved these changes

Assignees


No one assigned


Labels

Pythonsemver-major

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@joaocgreis@rvagg@cclauss@sam-github@bnoordhuis@richardlau

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Accept Python 3 by default - #1844
Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Closed
Accept Python 3 by default#1844
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Conversation
@joaocgreis
@joaocgreisjoaocgreis
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Checklist
Description of change
This depends on #1843, I will rebase when that PR lands. Only a971947 needs to be reviewed here.
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
@nodejs/node-gyp please not that we cover little more than a basic hello world in our tests, there are probably parts of Gyp that are not covered and are not compatible with Python 3. We will have to be extra careful shipping the node-gyp version that includes this.
@joaocgreis
joaocgreis
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/152/
richardlau
richardlau
reviewed
Jul 23, 2019
View reviewed changes
Comment threadlib/find-python.js
cclauss
cclauss
approved these changes
Jul 23, 2019
View reviewed changes
@cclausscclauss
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Awesome work here!
Can you please add a Python 3 on macOS on Travis CI test like #1846 just to make sure that is passing?
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
@cclauss I think #1846 is the proper place to do it, this PR might take a while to land. I'll rebase on top of that PR and adjust accordingly when it lands!
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
This is blocked by #1846, we can't land this with OSX failing.
@cclausscclauss
mentioned this pull request
Jul 24, 2019
 Closed

4 tasks
@rvagg
rvagg
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
seems good to me but I'm confused on whether the macos problems block this and #1843 or if they can land independently of that?
@cclauss
cclauss
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
We can land this with the macOS tests still running in allow_failures mode. Our README clearly states that Python 3 is not supported which aligns with Travis CI's intent for allow_failures
[allow_failures] lets you add in experimental and preparatory builds to test against versions or configurations that you are not ready to officially support.
@joaocgreis
joaocgreis
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
#1843 has landed. In there, node-gyp only uses Python 3 if EXPERIMENTAL_NODE_GYP_PYTHON3 is defined, so it should not break anything we currently have.
This PR removes that flag, making node-gyp accept Python 3. So, if a release is shipped with this PR, node-gyp will start using Python 3 in the users computers, and any issue we have will manifest there.
Currently, we know Python 3 is failing on osx, so shipping a release with this would break node-gyp on every osx machine that has Python 3 installed.
We know that our tests pass with Python 3 on Windows and Linux, but our tests don't have a good coverage of gyp. It's likely that there are modules out there that use parts of gyp that we don't test here, and those would be broken as well. GYP3 could help a lot with this.
So, we can't land this PR until all failures that we know of are fixed.
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9be02bc
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
9be02bc to
9318e21CompareJuly 26, 2019 15:19
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9318e21
PR-URL: nodejs#1844
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
b154b07
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
9318e21 to
b154b07CompareJuly 26, 2019 15:27
@sam-githubsam-github
mentioned this pull request
Sep 6, 2019

Closed

@sam-github
sam-github
commented
Sep 6, 2019

Copy link


Copy Markdown

Contributor
nodejs/node#29246 bumped into this.
I'm not sure what the plan is. If I'm missing the conversation elsewhere, pls point me there! I don't want to fragment an existing conversation.
node's tests can't pass on a py3-only machine until npm has a copy of node-gyp that supports py3.
Does npm even get node-gyp from this repo? :-( Sorry, you can see how late to this "party" I'm arriving.
@bnoordhuis
bnoordhuis
commented
Sep 6, 2019

Copy link


Copy Markdown

Member
Does npm even get node-gyp from this repo?
Yes. :-)
@joaocgreis
joaocgreis
commented
Sep 10, 2019

Copy link


Copy Markdown

MemberAuthor
@sam-github my comment above is still a valid summary of the current situation. Going forward, I believe the options on the table are either to use GYP3 (#1845) or fix the osx issues (@cclauss might be working on that).
This PR is ready to be rebased and land landed when we believe Python 3 support to be ready. However, this PR does not change any Python code, it only removes the block to run on Python 3.
@cclausscclauss
mentioned this pull request
Sep 11, 2019

Closed

@madacolmadacol
mentioned this pull request
Sep 12, 2019

Closed


This was referenced Sep 23, 2019

Closed

 Closed


Closed

joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 25, 2019
@joaocgreis
lib: accept Python 3 by default
f7a28c3
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
b154b07 to
f7a28c3CompareSeptember 25, 2019 13:39
@joaocgreis
joaocgreis
commented
Sep 25, 2019

Copy link


Copy Markdown

MemberAuthor
Rebased to make Travis run again, but it's still red. Current status: #1844 (comment) and #1844 (comment)
@cclauss
cclauss
commented
Sep 25, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The macOS error looks related to #1854 so I will attack it...
Fixed in Decode stdout on Python 3#1890
cclauss added a commit
to cclauss/node-gyp
that referenced
this pull request
Sep 25, 2019
@cclauss
Testnodejs#1844andnodejs#1854
9f89138

This was referenced Sep 25, 2019
 Closed

 Closed

@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Please rebase again to resolve conflict and ensure macOS now passes.
@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
  • On a machine that has both Python 2 and Python 3 installed?
  • On a machine that only has Python 3 installed?
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Sep 26, 2019
@joaocgreis
lib: accept Python 3 by default
a490ca5
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
f7a28c3 to
a490ca5CompareSeptember 26, 2019 13:09
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
1276037
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
a490ca5 to
1276037CompareSeptember 30, 2019 05:28
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3066b03
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
1276037 to
3066b03CompareSeptember 30, 2019 05:47
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
167b563
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
0ffb1aa to
167b563CompareSeptember 30, 2019 06:43
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
167b563 to
05138c8CompareSeptember 30, 2019 06:47
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
Doesn't "have" to, but I'd rather keep it in most (if not all) of the entries in Travis, since not having it was the reason we didn't know what Python was actually being run before. This is a good way to avoid surprises.
On a machine that has both Python 2 and Python 3 installed?

On a machine that only has Python 3 installed?
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2. Whatever exists first is what's used.
joaocgreis added a commit
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3d1c60a
PR-URL: #1844
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Hit an issue because of isaacs/minizlib#9 - Travis kept 1.3.1 in cache. Should be fixed now, forced update in one run and Travis updated the cache.
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/155/ ✔️
Landed in 3d1c60a
@sam-github
sam-github
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Does this change which version of python is chosen? I can't tell from the diff.
I.e
  • with python2, python3, and python (2.7) in /usr/bin, which will get chosen?
  • with python2, python3, and python (3.6) in /usr/bin, which will get chosen?
I don't have any concerns either way, but I think we should call out in the docs/changelog if the order of choice changed, right now it just says Python 3 is accepted, not under which conditions is preferred, if ever.
@cclauss
cclauss
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2.

Whatever exists first is what's used.
I take that to mean python will be used.
@sam-github
sam-github
commented
Oct 3, 2019

Copy link


Copy Markdown

Contributor
This makes node-gyp accept Python 3 and look for it before looking for Python 2, making Python 3 the default.
@joaocgreis Can I edit this description? I think that isn't quite accurate, it should be
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
Personally, I suggest that this PR can and should land on 5.x, with the minor tweek of moving python3 to afterpython2, so that users will Python 2 if they have it.
cf #1892 (comment) and previous discussion
@sam-githubsam-github
mentioned this pull request
Oct 3, 2019
 Closed

4 tasks
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
4215054
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
07f81f1
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@neilshneilsh
mentioned this pull request
Oct 28, 2019
 Merged

@rvaggrvagg
mentioned this pull request
Nov 18, 2019
 Closed

@rvaggrvagg
mentioned this pull request
Feb 3, 2020
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@cclausscclausscclauss approved these changes

Assignees


No one assigned


Labels

Pythonsemver-major

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@joaocgreis@rvagg@cclauss@sam-github@bnoordhuis@richardlau

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Accept Python 3 by default - #1844
Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Closed
Accept Python 3 by default#1844
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Conversation
@joaocgreis
@joaocgreisjoaocgreis
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Checklist
Description of change
This depends on #1843, I will rebase when that PR lands. Only a971947 needs to be reviewed here.
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
@nodejs/node-gyp please not that we cover little more than a basic hello world in our tests, there are probably parts of Gyp that are not covered and are not compatible with Python 3. We will have to be extra careful shipping the node-gyp version that includes this.
@joaocgreis
joaocgreis
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/152/
richardlau
richardlau
reviewed
Jul 23, 2019
View reviewed changes
Comment threadlib/find-python.js
cclauss
cclauss
approved these changes
Jul 23, 2019
View reviewed changes
@cclausscclauss
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Awesome work here!
Can you please add a Python 3 on macOS on Travis CI test like #1846 just to make sure that is passing?
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
@cclauss I think #1846 is the proper place to do it, this PR might take a while to land. I'll rebase on top of that PR and adjust accordingly when it lands!
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
This is blocked by #1846, we can't land this with OSX failing.
@cclausscclauss
mentioned this pull request
Jul 24, 2019
 Closed

4 tasks
@rvagg
rvagg
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
seems good to me but I'm confused on whether the macos problems block this and #1843 or if they can land independently of that?
@cclauss
cclauss
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
We can land this with the macOS tests still running in allow_failures mode. Our README clearly states that Python 3 is not supported which aligns with Travis CI's intent for allow_failures
[allow_failures] lets you add in experimental and preparatory builds to test against versions or configurations that you are not ready to officially support.
@joaocgreis
joaocgreis
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
#1843 has landed. In there, node-gyp only uses Python 3 if EXPERIMENTAL_NODE_GYP_PYTHON3 is defined, so it should not break anything we currently have.
This PR removes that flag, making node-gyp accept Python 3. So, if a release is shipped with this PR, node-gyp will start using Python 3 in the users computers, and any issue we have will manifest there.
Currently, we know Python 3 is failing on osx, so shipping a release with this would break node-gyp on every osx machine that has Python 3 installed.
We know that our tests pass with Python 3 on Windows and Linux, but our tests don't have a good coverage of gyp. It's likely that there are modules out there that use parts of gyp that we don't test here, and those would be broken as well. GYP3 could help a lot with this.
So, we can't land this PR until all failures that we know of are fixed.
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9be02bc
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
9be02bc to
9318e21CompareJuly 26, 2019 15:19
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9318e21
PR-URL: nodejs#1844
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
b154b07
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
9318e21 to
b154b07CompareJuly 26, 2019 15:27
@sam-githubsam-github
mentioned this pull request
Sep 6, 2019

Closed

@sam-github
sam-github
commented
Sep 6, 2019

Copy link


Copy Markdown

Contributor
nodejs/node#29246 bumped into this.
I'm not sure what the plan is. If I'm missing the conversation elsewhere, pls point me there! I don't want to fragment an existing conversation.
node's tests can't pass on a py3-only machine until npm has a copy of node-gyp that supports py3.
Does npm even get node-gyp from this repo? :-( Sorry, you can see how late to this "party" I'm arriving.
@bnoordhuis
bnoordhuis
commented
Sep 6, 2019

Copy link


Copy Markdown

Member
Does npm even get node-gyp from this repo?
Yes. :-)
@joaocgreis
joaocgreis
commented
Sep 10, 2019

Copy link


Copy Markdown

MemberAuthor
@sam-github my comment above is still a valid summary of the current situation. Going forward, I believe the options on the table are either to use GYP3 (#1845) or fix the osx issues (@cclauss might be working on that).
This PR is ready to be rebased and land landed when we believe Python 3 support to be ready. However, this PR does not change any Python code, it only removes the block to run on Python 3.
@cclausscclauss
mentioned this pull request
Sep 11, 2019

Closed

@madacolmadacol
mentioned this pull request
Sep 12, 2019

Closed


This was referenced Sep 23, 2019

Closed

 Closed


Closed

joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 25, 2019
@joaocgreis
lib: accept Python 3 by default
f7a28c3
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
b154b07 to
f7a28c3CompareSeptember 25, 2019 13:39
@joaocgreis
joaocgreis
commented
Sep 25, 2019

Copy link


Copy Markdown

MemberAuthor
Rebased to make Travis run again, but it's still red. Current status: #1844 (comment) and #1844 (comment)
@cclauss
cclauss
commented
Sep 25, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The macOS error looks related to #1854 so I will attack it...
Fixed in Decode stdout on Python 3#1890
cclauss added a commit
to cclauss/node-gyp
that referenced
this pull request
Sep 25, 2019
@cclauss
Testnodejs#1844andnodejs#1854
9f89138

This was referenced Sep 25, 2019
 Closed

 Closed

@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Please rebase again to resolve conflict and ensure macOS now passes.
@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
  • On a machine that has both Python 2 and Python 3 installed?
  • On a machine that only has Python 3 installed?
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Sep 26, 2019
@joaocgreis
lib: accept Python 3 by default
a490ca5
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
f7a28c3 to
a490ca5CompareSeptember 26, 2019 13:09
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
1276037
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
a490ca5 to
1276037CompareSeptember 30, 2019 05:28
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3066b03
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
1276037 to
3066b03CompareSeptember 30, 2019 05:47
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
167b563
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
0ffb1aa to
167b563CompareSeptember 30, 2019 06:43
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
167b563 to
05138c8CompareSeptember 30, 2019 06:47
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
Doesn't "have" to, but I'd rather keep it in most (if not all) of the entries in Travis, since not having it was the reason we didn't know what Python was actually being run before. This is a good way to avoid surprises.
On a machine that has both Python 2 and Python 3 installed?

On a machine that only has Python 3 installed?
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2. Whatever exists first is what's used.
joaocgreis added a commit
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3d1c60a
PR-URL: #1844
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Hit an issue because of isaacs/minizlib#9 - Travis kept 1.3.1 in cache. Should be fixed now, forced update in one run and Travis updated the cache.
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/155/ ✔️
Landed in 3d1c60a
@sam-github
sam-github
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Does this change which version of python is chosen? I can't tell from the diff.
I.e
  • with python2, python3, and python (2.7) in /usr/bin, which will get chosen?
  • with python2, python3, and python (3.6) in /usr/bin, which will get chosen?
I don't have any concerns either way, but I think we should call out in the docs/changelog if the order of choice changed, right now it just says Python 3 is accepted, not under which conditions is preferred, if ever.
@cclauss
cclauss
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2.

Whatever exists first is what's used.
I take that to mean python will be used.
@sam-github
sam-github
commented
Oct 3, 2019

Copy link


Copy Markdown

Contributor
This makes node-gyp accept Python 3 and look for it before looking for Python 2, making Python 3 the default.
@joaocgreis Can I edit this description? I think that isn't quite accurate, it should be
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
Personally, I suggest that this PR can and should land on 5.x, with the minor tweek of moving python3 to afterpython2, so that users will Python 2 if they have it.
cf #1892 (comment) and previous discussion
@sam-githubsam-github
mentioned this pull request
Oct 3, 2019
 Closed

4 tasks
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
4215054
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
07f81f1
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@neilshneilsh
mentioned this pull request
Oct 28, 2019
 Merged

@rvaggrvagg
mentioned this pull request
Nov 18, 2019
 Closed

@rvaggrvagg
mentioned this pull request
Feb 3, 2020
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@cclausscclausscclauss approved these changes

Assignees


No one assigned


Labels

Pythonsemver-major

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@joaocgreis@rvagg@cclauss@sam-github@bnoordhuis@richardlau

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Accept Python 3 by default - #1844
Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Closed
Accept Python 3 by default#1844
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Conversation
@joaocgreis
@joaocgreisjoaocgreis
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Checklist
Description of change
This depends on #1843, I will rebase when that PR lands. Only a971947 needs to be reviewed here.
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
@nodejs/node-gyp please not that we cover little more than a basic hello world in our tests, there are probably parts of Gyp that are not covered and are not compatible with Python 3. We will have to be extra careful shipping the node-gyp version that includes this.
@joaocgreis
joaocgreis
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/152/
richardlau
richardlau
reviewed
Jul 23, 2019
View reviewed changes
Comment threadlib/find-python.js
cclauss
cclauss
approved these changes
Jul 23, 2019
View reviewed changes
@cclausscclauss
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Awesome work here!
Can you please add a Python 3 on macOS on Travis CI test like #1846 just to make sure that is passing?
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
@cclauss I think #1846 is the proper place to do it, this PR might take a while to land. I'll rebase on top of that PR and adjust accordingly when it lands!
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
This is blocked by #1846, we can't land this with OSX failing.
@cclausscclauss
mentioned this pull request
Jul 24, 2019
 Closed

4 tasks
@rvagg
rvagg
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
seems good to me but I'm confused on whether the macos problems block this and #1843 or if they can land independently of that?
@cclauss
cclauss
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
We can land this with the macOS tests still running in allow_failures mode. Our README clearly states that Python 3 is not supported which aligns with Travis CI's intent for allow_failures
[allow_failures] lets you add in experimental and preparatory builds to test against versions or configurations that you are not ready to officially support.
@joaocgreis
joaocgreis
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
#1843 has landed. In there, node-gyp only uses Python 3 if EXPERIMENTAL_NODE_GYP_PYTHON3 is defined, so it should not break anything we currently have.
This PR removes that flag, making node-gyp accept Python 3. So, if a release is shipped with this PR, node-gyp will start using Python 3 in the users computers, and any issue we have will manifest there.
Currently, we know Python 3 is failing on osx, so shipping a release with this would break node-gyp on every osx machine that has Python 3 installed.
We know that our tests pass with Python 3 on Windows and Linux, but our tests don't have a good coverage of gyp. It's likely that there are modules out there that use parts of gyp that we don't test here, and those would be broken as well. GYP3 could help a lot with this.
So, we can't land this PR until all failures that we know of are fixed.
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9be02bc
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
9be02bc to
9318e21CompareJuly 26, 2019 15:19
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9318e21
PR-URL: nodejs#1844
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
b154b07
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
9318e21 to
b154b07CompareJuly 26, 2019 15:27
@sam-githubsam-github
mentioned this pull request
Sep 6, 2019

Closed

@sam-github
sam-github
commented
Sep 6, 2019

Copy link


Copy Markdown

Contributor
nodejs/node#29246 bumped into this.
I'm not sure what the plan is. If I'm missing the conversation elsewhere, pls point me there! I don't want to fragment an existing conversation.
node's tests can't pass on a py3-only machine until npm has a copy of node-gyp that supports py3.
Does npm even get node-gyp from this repo? :-( Sorry, you can see how late to this "party" I'm arriving.
@bnoordhuis
bnoordhuis
commented
Sep 6, 2019

Copy link


Copy Markdown

Member
Does npm even get node-gyp from this repo?
Yes. :-)
@joaocgreis
joaocgreis
commented
Sep 10, 2019

Copy link


Copy Markdown

MemberAuthor
@sam-github my comment above is still a valid summary of the current situation. Going forward, I believe the options on the table are either to use GYP3 (#1845) or fix the osx issues (@cclauss might be working on that).
This PR is ready to be rebased and land landed when we believe Python 3 support to be ready. However, this PR does not change any Python code, it only removes the block to run on Python 3.
@cclausscclauss
mentioned this pull request
Sep 11, 2019

Closed

@madacolmadacol
mentioned this pull request
Sep 12, 2019

Closed


This was referenced Sep 23, 2019

Closed

 Closed


Closed

joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 25, 2019
@joaocgreis
lib: accept Python 3 by default
f7a28c3
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
b154b07 to
f7a28c3CompareSeptember 25, 2019 13:39
@joaocgreis
joaocgreis
commented
Sep 25, 2019

Copy link


Copy Markdown

MemberAuthor
Rebased to make Travis run again, but it's still red. Current status: #1844 (comment) and #1844 (comment)
@cclauss
cclauss
commented
Sep 25, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The macOS error looks related to #1854 so I will attack it...
Fixed in Decode stdout on Python 3#1890
cclauss added a commit
to cclauss/node-gyp
that referenced
this pull request
Sep 25, 2019
@cclauss
Testnodejs#1844andnodejs#1854
9f89138

This was referenced Sep 25, 2019
 Closed

 Closed

@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Please rebase again to resolve conflict and ensure macOS now passes.
@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
  • On a machine that has both Python 2 and Python 3 installed?
  • On a machine that only has Python 3 installed?
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Sep 26, 2019
@joaocgreis
lib: accept Python 3 by default
a490ca5
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
f7a28c3 to
a490ca5CompareSeptember 26, 2019 13:09
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
1276037
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
a490ca5 to
1276037CompareSeptember 30, 2019 05:28
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3066b03
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
1276037 to
3066b03CompareSeptember 30, 2019 05:47
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
167b563
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
0ffb1aa to
167b563CompareSeptember 30, 2019 06:43
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
167b563 to
05138c8CompareSeptember 30, 2019 06:47
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
Doesn't "have" to, but I'd rather keep it in most (if not all) of the entries in Travis, since not having it was the reason we didn't know what Python was actually being run before. This is a good way to avoid surprises.
On a machine that has both Python 2 and Python 3 installed?

On a machine that only has Python 3 installed?
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2. Whatever exists first is what's used.
joaocgreis added a commit
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3d1c60a
PR-URL: #1844
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Hit an issue because of isaacs/minizlib#9 - Travis kept 1.3.1 in cache. Should be fixed now, forced update in one run and Travis updated the cache.
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/155/ ✔️
Landed in 3d1c60a
@sam-github
sam-github
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Does this change which version of python is chosen? I can't tell from the diff.
I.e
  • with python2, python3, and python (2.7) in /usr/bin, which will get chosen?
  • with python2, python3, and python (3.6) in /usr/bin, which will get chosen?
I don't have any concerns either way, but I think we should call out in the docs/changelog if the order of choice changed, right now it just says Python 3 is accepted, not under which conditions is preferred, if ever.
@cclauss
cclauss
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2.

Whatever exists first is what's used.
I take that to mean python will be used.
@sam-github
sam-github
commented
Oct 3, 2019

Copy link


Copy Markdown

Contributor
This makes node-gyp accept Python 3 and look for it before looking for Python 2, making Python 3 the default.
@joaocgreis Can I edit this description? I think that isn't quite accurate, it should be
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
Personally, I suggest that this PR can and should land on 5.x, with the minor tweek of moving python3 to afterpython2, so that users will Python 2 if they have it.
cf #1892 (comment) and previous discussion
@sam-githubsam-github
mentioned this pull request
Oct 3, 2019
 Closed

4 tasks
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
4215054
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
07f81f1
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@neilshneilsh
mentioned this pull request
Oct 28, 2019
 Merged

@rvaggrvagg
mentioned this pull request
Nov 18, 2019
 Closed

@rvaggrvagg
mentioned this pull request
Feb 3, 2020
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@cclausscclausscclauss approved these changes

Assignees


No one assigned


Labels

Pythonsemver-major

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@joaocgreis@rvagg@cclauss@sam-github@bnoordhuis@richardlau

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Accept Python 3 by default - #1844
Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Closed
Accept Python 3 by default#1844
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Conversation
@joaocgreis
@joaocgreisjoaocgreis
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Checklist
Description of change
This depends on #1843, I will rebase when that PR lands. Only a971947 needs to be reviewed here.
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
@nodejs/node-gyp please not that we cover little more than a basic hello world in our tests, there are probably parts of Gyp that are not covered and are not compatible with Python 3. We will have to be extra careful shipping the node-gyp version that includes this.
@joaocgreis
joaocgreis
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/152/
richardlau
richardlau
reviewed
Jul 23, 2019
View reviewed changes
Comment threadlib/find-python.js
cclauss
cclauss
approved these changes
Jul 23, 2019
View reviewed changes
@cclausscclauss
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Awesome work here!
Can you please add a Python 3 on macOS on Travis CI test like #1846 just to make sure that is passing?
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
@cclauss I think #1846 is the proper place to do it, this PR might take a while to land. I'll rebase on top of that PR and adjust accordingly when it lands!
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
This is blocked by #1846, we can't land this with OSX failing.
@cclausscclauss
mentioned this pull request
Jul 24, 2019
 Closed

4 tasks
@rvagg
rvagg
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
seems good to me but I'm confused on whether the macos problems block this and #1843 or if they can land independently of that?
@cclauss
cclauss
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
We can land this with the macOS tests still running in allow_failures mode. Our README clearly states that Python 3 is not supported which aligns with Travis CI's intent for allow_failures
[allow_failures] lets you add in experimental and preparatory builds to test against versions or configurations that you are not ready to officially support.
@joaocgreis
joaocgreis
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
#1843 has landed. In there, node-gyp only uses Python 3 if EXPERIMENTAL_NODE_GYP_PYTHON3 is defined, so it should not break anything we currently have.
This PR removes that flag, making node-gyp accept Python 3. So, if a release is shipped with this PR, node-gyp will start using Python 3 in the users computers, and any issue we have will manifest there.
Currently, we know Python 3 is failing on osx, so shipping a release with this would break node-gyp on every osx machine that has Python 3 installed.
We know that our tests pass with Python 3 on Windows and Linux, but our tests don't have a good coverage of gyp. It's likely that there are modules out there that use parts of gyp that we don't test here, and those would be broken as well. GYP3 could help a lot with this.
So, we can't land this PR until all failures that we know of are fixed.
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9be02bc
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
9be02bc to
9318e21CompareJuly 26, 2019 15:19
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9318e21
PR-URL: nodejs#1844
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
b154b07
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
9318e21 to
b154b07CompareJuly 26, 2019 15:27
@sam-githubsam-github
mentioned this pull request
Sep 6, 2019

Closed

@sam-github
sam-github
commented
Sep 6, 2019

Copy link


Copy Markdown

Contributor
nodejs/node#29246 bumped into this.
I'm not sure what the plan is. If I'm missing the conversation elsewhere, pls point me there! I don't want to fragment an existing conversation.
node's tests can't pass on a py3-only machine until npm has a copy of node-gyp that supports py3.
Does npm even get node-gyp from this repo? :-( Sorry, you can see how late to this "party" I'm arriving.
@bnoordhuis
bnoordhuis
commented
Sep 6, 2019

Copy link


Copy Markdown

Member
Does npm even get node-gyp from this repo?
Yes. :-)
@joaocgreis
joaocgreis
commented
Sep 10, 2019

Copy link


Copy Markdown

MemberAuthor
@sam-github my comment above is still a valid summary of the current situation. Going forward, I believe the options on the table are either to use GYP3 (#1845) or fix the osx issues (@cclauss might be working on that).
This PR is ready to be rebased and land landed when we believe Python 3 support to be ready. However, this PR does not change any Python code, it only removes the block to run on Python 3.
@cclausscclauss
mentioned this pull request
Sep 11, 2019

Closed

@madacolmadacol
mentioned this pull request
Sep 12, 2019

Closed


This was referenced Sep 23, 2019

Closed

 Closed


Closed

joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 25, 2019
@joaocgreis
lib: accept Python 3 by default
f7a28c3
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
b154b07 to
f7a28c3CompareSeptember 25, 2019 13:39
@joaocgreis
joaocgreis
commented
Sep 25, 2019

Copy link


Copy Markdown

MemberAuthor
Rebased to make Travis run again, but it's still red. Current status: #1844 (comment) and #1844 (comment)
@cclauss
cclauss
commented
Sep 25, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The macOS error looks related to #1854 so I will attack it...
Fixed in Decode stdout on Python 3#1890
cclauss added a commit
to cclauss/node-gyp
that referenced
this pull request
Sep 25, 2019
@cclauss
Testnodejs#1844andnodejs#1854
9f89138

This was referenced Sep 25, 2019
 Closed

 Closed

@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Please rebase again to resolve conflict and ensure macOS now passes.
@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
  • On a machine that has both Python 2 and Python 3 installed?
  • On a machine that only has Python 3 installed?
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Sep 26, 2019
@joaocgreis
lib: accept Python 3 by default
a490ca5
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
f7a28c3 to
a490ca5CompareSeptember 26, 2019 13:09
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
1276037
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
a490ca5 to
1276037CompareSeptember 30, 2019 05:28
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3066b03
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
1276037 to
3066b03CompareSeptember 30, 2019 05:47
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
167b563
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
0ffb1aa to
167b563CompareSeptember 30, 2019 06:43
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
167b563 to
05138c8CompareSeptember 30, 2019 06:47
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
Doesn't "have" to, but I'd rather keep it in most (if not all) of the entries in Travis, since not having it was the reason we didn't know what Python was actually being run before. This is a good way to avoid surprises.
On a machine that has both Python 2 and Python 3 installed?

On a machine that only has Python 3 installed?
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2. Whatever exists first is what's used.
joaocgreis added a commit
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3d1c60a
PR-URL: #1844
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Hit an issue because of isaacs/minizlib#9 - Travis kept 1.3.1 in cache. Should be fixed now, forced update in one run and Travis updated the cache.
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/155/ ✔️
Landed in 3d1c60a
@sam-github
sam-github
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Does this change which version of python is chosen? I can't tell from the diff.
I.e
  • with python2, python3, and python (2.7) in /usr/bin, which will get chosen?
  • with python2, python3, and python (3.6) in /usr/bin, which will get chosen?
I don't have any concerns either way, but I think we should call out in the docs/changelog if the order of choice changed, right now it just says Python 3 is accepted, not under which conditions is preferred, if ever.
@cclauss
cclauss
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2.

Whatever exists first is what's used.
I take that to mean python will be used.
@sam-github
sam-github
commented
Oct 3, 2019

Copy link


Copy Markdown

Contributor
This makes node-gyp accept Python 3 and look for it before looking for Python 2, making Python 3 the default.
@joaocgreis Can I edit this description? I think that isn't quite accurate, it should be
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
Personally, I suggest that this PR can and should land on 5.x, with the minor tweek of moving python3 to afterpython2, so that users will Python 2 if they have it.
cf #1892 (comment) and previous discussion
@sam-githubsam-github
mentioned this pull request
Oct 3, 2019
 Closed

4 tasks
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
4215054
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
07f81f1
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@neilshneilsh
mentioned this pull request
Oct 28, 2019
 Merged

@rvaggrvagg
mentioned this pull request
Nov 18, 2019
 Closed

@rvaggrvagg
mentioned this pull request
Feb 3, 2020
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@cclausscclausscclauss approved these changes

Assignees


No one assigned


Labels

Pythonsemver-major

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@joaocgreis@rvagg@cclauss@sam-github@bnoordhuis@richardlau

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Accept Python 3 by default - #1844
Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Closed
Accept Python 3 by default#1844
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Conversation
@joaocgreis
@joaocgreisjoaocgreis
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Checklist
Description of change
This depends on #1843, I will rebase when that PR lands. Only a971947 needs to be reviewed here.
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
@nodejs/node-gyp please not that we cover little more than a basic hello world in our tests, there are probably parts of Gyp that are not covered and are not compatible with Python 3. We will have to be extra careful shipping the node-gyp version that includes this.
@joaocgreis
joaocgreis
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/152/
richardlau
richardlau
reviewed
Jul 23, 2019
View reviewed changes
Comment threadlib/find-python.js
cclauss
cclauss
approved these changes
Jul 23, 2019
View reviewed changes
@cclausscclauss
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Awesome work here!
Can you please add a Python 3 on macOS on Travis CI test like #1846 just to make sure that is passing?
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
@cclauss I think #1846 is the proper place to do it, this PR might take a while to land. I'll rebase on top of that PR and adjust accordingly when it lands!
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
This is blocked by #1846, we can't land this with OSX failing.
@cclausscclauss
mentioned this pull request
Jul 24, 2019
 Closed

4 tasks
@rvagg
rvagg
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
seems good to me but I'm confused on whether the macos problems block this and #1843 or if they can land independently of that?
@cclauss
cclauss
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
We can land this with the macOS tests still running in allow_failures mode. Our README clearly states that Python 3 is not supported which aligns with Travis CI's intent for allow_failures
[allow_failures] lets you add in experimental and preparatory builds to test against versions or configurations that you are not ready to officially support.
@joaocgreis
joaocgreis
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
#1843 has landed. In there, node-gyp only uses Python 3 if EXPERIMENTAL_NODE_GYP_PYTHON3 is defined, so it should not break anything we currently have.
This PR removes that flag, making node-gyp accept Python 3. So, if a release is shipped with this PR, node-gyp will start using Python 3 in the users computers, and any issue we have will manifest there.
Currently, we know Python 3 is failing on osx, so shipping a release with this would break node-gyp on every osx machine that has Python 3 installed.
We know that our tests pass with Python 3 on Windows and Linux, but our tests don't have a good coverage of gyp. It's likely that there are modules out there that use parts of gyp that we don't test here, and those would be broken as well. GYP3 could help a lot with this.
So, we can't land this PR until all failures that we know of are fixed.
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9be02bc
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
9be02bc to
9318e21CompareJuly 26, 2019 15:19
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9318e21
PR-URL: nodejs#1844
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
b154b07
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
9318e21 to
b154b07CompareJuly 26, 2019 15:27
@sam-githubsam-github
mentioned this pull request
Sep 6, 2019

Closed

@sam-github
sam-github
commented
Sep 6, 2019

Copy link


Copy Markdown

Contributor
nodejs/node#29246 bumped into this.
I'm not sure what the plan is. If I'm missing the conversation elsewhere, pls point me there! I don't want to fragment an existing conversation.
node's tests can't pass on a py3-only machine until npm has a copy of node-gyp that supports py3.
Does npm even get node-gyp from this repo? :-( Sorry, you can see how late to this "party" I'm arriving.
@bnoordhuis
bnoordhuis
commented
Sep 6, 2019

Copy link


Copy Markdown

Member
Does npm even get node-gyp from this repo?
Yes. :-)
@joaocgreis
joaocgreis
commented
Sep 10, 2019

Copy link


Copy Markdown

MemberAuthor
@sam-github my comment above is still a valid summary of the current situation. Going forward, I believe the options on the table are either to use GYP3 (#1845) or fix the osx issues (@cclauss might be working on that).
This PR is ready to be rebased and land landed when we believe Python 3 support to be ready. However, this PR does not change any Python code, it only removes the block to run on Python 3.
@cclausscclauss
mentioned this pull request
Sep 11, 2019

Closed

@madacolmadacol
mentioned this pull request
Sep 12, 2019

Closed


This was referenced Sep 23, 2019

Closed

 Closed


Closed

joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 25, 2019
@joaocgreis
lib: accept Python 3 by default
f7a28c3
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
b154b07 to
f7a28c3CompareSeptember 25, 2019 13:39
@joaocgreis
joaocgreis
commented
Sep 25, 2019

Copy link


Copy Markdown

MemberAuthor
Rebased to make Travis run again, but it's still red. Current status: #1844 (comment) and #1844 (comment)
@cclauss
cclauss
commented
Sep 25, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The macOS error looks related to #1854 so I will attack it...
Fixed in Decode stdout on Python 3#1890
cclauss added a commit
to cclauss/node-gyp
that referenced
this pull request
Sep 25, 2019
@cclauss
Testnodejs#1844andnodejs#1854
9f89138

This was referenced Sep 25, 2019
 Closed

 Closed

@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Please rebase again to resolve conflict and ensure macOS now passes.
@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
  • On a machine that has both Python 2 and Python 3 installed?
  • On a machine that only has Python 3 installed?
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Sep 26, 2019
@joaocgreis
lib: accept Python 3 by default
a490ca5
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
f7a28c3 to
a490ca5CompareSeptember 26, 2019 13:09
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
1276037
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
a490ca5 to
1276037CompareSeptember 30, 2019 05:28
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3066b03
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
1276037 to
3066b03CompareSeptember 30, 2019 05:47
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
167b563
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
0ffb1aa to
167b563CompareSeptember 30, 2019 06:43
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
167b563 to
05138c8CompareSeptember 30, 2019 06:47
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
Doesn't "have" to, but I'd rather keep it in most (if not all) of the entries in Travis, since not having it was the reason we didn't know what Python was actually being run before. This is a good way to avoid surprises.
On a machine that has both Python 2 and Python 3 installed?

On a machine that only has Python 3 installed?
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2. Whatever exists first is what's used.
joaocgreis added a commit
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3d1c60a
PR-URL: #1844
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Hit an issue because of isaacs/minizlib#9 - Travis kept 1.3.1 in cache. Should be fixed now, forced update in one run and Travis updated the cache.
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/155/ ✔️
Landed in 3d1c60a
@sam-github
sam-github
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Does this change which version of python is chosen? I can't tell from the diff.
I.e
  • with python2, python3, and python (2.7) in /usr/bin, which will get chosen?
  • with python2, python3, and python (3.6) in /usr/bin, which will get chosen?
I don't have any concerns either way, but I think we should call out in the docs/changelog if the order of choice changed, right now it just says Python 3 is accepted, not under which conditions is preferred, if ever.
@cclauss
cclauss
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2.

Whatever exists first is what's used.
I take that to mean python will be used.
@sam-github
sam-github
commented
Oct 3, 2019

Copy link


Copy Markdown

Contributor
This makes node-gyp accept Python 3 and look for it before looking for Python 2, making Python 3 the default.
@joaocgreis Can I edit this description? I think that isn't quite accurate, it should be
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
Personally, I suggest that this PR can and should land on 5.x, with the minor tweek of moving python3 to afterpython2, so that users will Python 2 if they have it.
cf #1892 (comment) and previous discussion
@sam-githubsam-github
mentioned this pull request
Oct 3, 2019
 Closed

4 tasks
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
4215054
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
07f81f1
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@neilshneilsh
mentioned this pull request
Oct 28, 2019
 Merged

@rvaggrvagg
mentioned this pull request
Nov 18, 2019
 Closed

@rvaggrvagg
mentioned this pull request
Feb 3, 2020
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@cclausscclausscclauss approved these changes

Assignees


No one assigned


Labels

Pythonsemver-major

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@joaocgreis@rvagg@cclauss@sam-github@bnoordhuis@richardlau

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Accept Python 3 by default - #1844
Closed
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Closed
Accept Python 3 by default#1844
joaocgreis wants to merge 1 commit into
nodejs:masterfrom 
joaocgreis:joaocgreis-J7N-python3-3
Conversation
@joaocgreis
@joaocgreisjoaocgreis
commented
Jul 23, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Checklist
Description of change
This depends on #1843, I will rebase when that PR lands. Only a971947 needs to be reviewed here.
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
@nodejs/node-gyp please not that we cover little more than a basic hello world in our tests, there are probably parts of Gyp that are not covered and are not compatible with Python 3. We will have to be extra careful shipping the node-gyp version that includes this.
@joaocgreis
joaocgreis
commented
Jul 23, 2019

Copy link


Copy Markdown

MemberAuthor
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/152/
richardlau
richardlau
reviewed
Jul 23, 2019
View reviewed changes
Comment threadlib/find-python.js
cclauss
cclauss
approved these changes
Jul 23, 2019
View reviewed changes
@cclausscclauss
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Awesome work here!
Can you please add a Python 3 on macOS on Travis CI test like #1846 just to make sure that is passing?
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
@cclauss I think #1846 is the proper place to do it, this PR might take a while to land. I'll rebase on top of that PR and adjust accordingly when it lands!
@joaocgreis
joaocgreis
commented
Jul 24, 2019

Copy link


Copy Markdown

MemberAuthor
This is blocked by #1846, we can't land this with OSX failing.
@cclausscclauss
mentioned this pull request
Jul 24, 2019
 Closed

4 tasks
@rvagg
rvagg
commented
Jul 26, 2019

Copy link


Copy Markdown

Member
seems good to me but I'm confused on whether the macos problems block this and #1843 or if they can land independently of that?
@cclauss
cclauss
commented
Jul 26, 2019

Copy link


Copy Markdown

Contributor
We can land this with the macOS tests still running in allow_failures mode. Our README clearly states that Python 3 is not supported which aligns with Travis CI's intent for allow_failures
[allow_failures] lets you add in experimental and preparatory builds to test against versions or configurations that you are not ready to officially support.
@joaocgreis
joaocgreis
commented
Jul 26, 2019

Copy link


Copy Markdown

MemberAuthor
#1843 has landed. In there, node-gyp only uses Python 3 if EXPERIMENTAL_NODE_GYP_PYTHON3 is defined, so it should not break anything we currently have.
This PR removes that flag, making node-gyp accept Python 3. So, if a release is shipped with this PR, node-gyp will start using Python 3 in the users computers, and any issue we have will manifest there.
Currently, we know Python 3 is failing on osx, so shipping a release with this would break node-gyp on every osx machine that has Python 3 installed.
We know that our tests pass with Python 3 on Windows and Linux, but our tests don't have a good coverage of gyp. It's likely that there are modules out there that use parts of gyp that we don't test here, and those would be broken as well. GYP3 could help a lot with this.
So, we can't land this PR until all failures that we know of are fixed.
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9be02bc
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
9be02bc to
9318e21CompareJuly 26, 2019 15:19
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
9318e21
PR-URL: nodejs#1844
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Jul 26, 2019
@joaocgreis
lib: accept Python 3 by default
b154b07
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
9318e21 to
b154b07CompareJuly 26, 2019 15:27
@sam-githubsam-github
mentioned this pull request
Sep 6, 2019

Closed

@sam-github
sam-github
commented
Sep 6, 2019

Copy link


Copy Markdown

Contributor
nodejs/node#29246 bumped into this.
I'm not sure what the plan is. If I'm missing the conversation elsewhere, pls point me there! I don't want to fragment an existing conversation.
node's tests can't pass on a py3-only machine until npm has a copy of node-gyp that supports py3.
Does npm even get node-gyp from this repo? :-( Sorry, you can see how late to this "party" I'm arriving.
@bnoordhuis
bnoordhuis
commented
Sep 6, 2019

Copy link


Copy Markdown

Member
Does npm even get node-gyp from this repo?
Yes. :-)
@joaocgreis
joaocgreis
commented
Sep 10, 2019

Copy link


Copy Markdown

MemberAuthor
@sam-github my comment above is still a valid summary of the current situation. Going forward, I believe the options on the table are either to use GYP3 (#1845) or fix the osx issues (@cclauss might be working on that).
This PR is ready to be rebased and land landed when we believe Python 3 support to be ready. However, this PR does not change any Python code, it only removes the block to run on Python 3.
@cclausscclauss
mentioned this pull request
Sep 11, 2019

Closed

@madacolmadacol
mentioned this pull request
Sep 12, 2019

Closed


This was referenced Sep 23, 2019

Closed

 Closed


Closed

joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 25, 2019
@joaocgreis
lib: accept Python 3 by default
f7a28c3
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
b154b07 to
f7a28c3CompareSeptember 25, 2019 13:39
@joaocgreis
joaocgreis
commented
Sep 25, 2019

Copy link


Copy Markdown

MemberAuthor
Rebased to make Travis run again, but it's still red. Current status: #1844 (comment) and #1844 (comment)
@cclauss
cclauss
commented
Sep 25, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
The macOS error looks related to #1854 so I will attack it...
Fixed in Decode stdout on Python 3#1890
cclauss added a commit
to cclauss/node-gyp
that referenced
this pull request
Sep 25, 2019
@cclauss
Testnodejs#1844andnodejs#1854
9f89138

This was referenced Sep 25, 2019
 Closed

 Closed

@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Please rebase again to resolve conflict and ensure macOS now passes.
@cclauss
cclauss
commented
Sep 26, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
  • On a machine that has both Python 2 and Python 3 installed?
  • On a machine that only has Python 3 installed?
joaocgreis added a commit
to JaneaSystems/node-gyp
that referenced
this pull request
Sep 26, 2019
@joaocgreis
lib: accept Python 3 by default
a490ca5
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
f7a28c3 to
a490ca5CompareSeptember 26, 2019 13:09
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
1276037
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
a490ca5 to
1276037CompareSeptember 30, 2019 05:28
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3066b03
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
1276037 to
3066b03CompareSeptember 30, 2019 05:47
joaocgreis added a commit
to joaocgreis/node-gyp
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
167b563
PR-URL: nodejs#1844
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
2 times, most recently
from
0ffb1aa to
167b563CompareSeptember 30, 2019 06:43
@joaocgreis
joaocgreisforce-pushed
the
joaocgreis-J7N-python3-3
branch
from
167b563 to
05138c8CompareSeptember 30, 2019 06:47
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Once this PR lands, does NODE_GYP_FORCE_PYTHON=python3 still have to be set to use Py3?
Doesn't "have" to, but I'd rather keep it in most (if not all) of the entries in Travis, since not having it was the reason we didn't know what Python was actually being run before. This is a good way to avoid surprises.
On a machine that has both Python 2 and Python 3 installed?

On a machine that only has Python 3 installed?
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2. Whatever exists first is what's used.
joaocgreis added a commit
that referenced
this pull request
Sep 30, 2019
@joaocgreis
lib: accept Python 3 by default
3d1c60a
PR-URL: #1844
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis
joaocgreis
commented
Sep 30, 2019

Copy link


Copy Markdown

MemberAuthor
Hit an issue because of isaacs/minizlib#9 - Travis kept 1.3.1 in cache. Should be fixed now, forced update in one run and Travis updated the cache.
CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/155/ ✔️
Landed in 3d1c60a
@sam-github
sam-github
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Does this change which version of python is chosen? I can't tell from the diff.
I.e
  • with python2, python3, and python (2.7) in /usr/bin, which will get chosen?
  • with python2, python3, and python (3.6) in /usr/bin, which will get chosen?
I don't have any concerns either way, but I think we should call out in the docs/changelog if the order of choice changed, right now it just says Python 3 is accepted, not under which conditions is preferred, if ever.
@cclauss
cclauss
commented
Oct 2, 2019

Copy link


Copy Markdown

Contributor
Without NODE_GYP_FORCE_PYTHON the code looks for python, then python3, then python2.

Whatever exists first is what's used.
I take that to mean python will be used.
@sam-github
sam-github
commented
Oct 3, 2019

Copy link


Copy Markdown

Contributor
This makes node-gyp accept Python 3 and look for it before looking for Python 2, making Python 3 the default.
@joaocgreis Can I edit this description? I think that isn't quite accurate, it should be
This makes node-gyp accept Python 3 as compatible without needing EXPERIMENTAL_NODE_GYP_PYTHON3 in the environment. It also looks for python, python3, then python2 (in that order). Note that python is usually Python 2.
Personally, I suggest that this PR can and should land on 5.x, with the minor tweek of moving python3 to afterpython2, so that users will Python 2 if they have it.
cf #1892 (comment) and previous discussion
@sam-githubsam-github
mentioned this pull request
Oct 3, 2019
 Closed

4 tasks
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
4215054
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg pushed a commit
that referenced
this pull request
Oct 4, 2019
@sam-github@rvagg
lib: accept Python 3 after Python 2
07f81f1
Python 3 is allowed as a compatible Python, but its looked for after
Python 2.
rvagg: removed additional EXPERIMENTAL_NODE_GYP_PYTHON3 in
.travis.yml from a previous commit while landing.
Backport-of: #1844
PR-URL: #1910
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: João Reis <reis@janeasystems.com>
@neilshneilsh
mentioned this pull request
Oct 28, 2019
 Merged

@rvaggrvagg
mentioned this pull request
Nov 18, 2019
 Closed

@rvaggrvagg
mentioned this pull request
Feb 3, 2020
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@cclausscclausscclauss approved these changes

Assignees


No one assigned


Labels

Pythonsemver-major

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@joaocgreis@rvagg@cclauss@sam-github@bnoordhuis@richardlau