build: update required python version to 3.8 #54358

RedYetiDev · 2024-08-13T16:27:21Z

Configure scripts use := operator now, which requires python 3.8.

File "tools/gyp/pylib/gyp/common.py", line 435 if CC := os.environ.get("CC_target") or os.environ.get("CC"):

Co-Authored-By: Jim B <[email protected]>

RedYetiDev · 2024-08-13T17:18:40Z

CC @nodejs/python @nodejs/build

CC previous participants @d3x0r @lpinca

cclauss · 2024-08-13T19:55:32Z

Instead of doing this over and over again, I would strongly recommend that we point readers to the currently supported versions as we do at https://github.com/nodejs/node-gyp/blob/main/README.md#on-unix

android-configure

cclauss

Were my comments unclear?

RedYetiDev · 2024-08-13T20:10:45Z

Instead of doing this over and over again, I would strongly recommend that we point readers to the currently supported versions as we do at nodejs/node-gyp@main/README.md#on-unix

I've made that change to the "Windows" section, but I think it's okay to keep the exact version in "Prerequisites". As for 3.12, I think that's an issue for a different PR, as this one is specifically removing up until 3.8, not adding more versions.

RedYetiDev · 2024-08-13T20:32:47Z

@cclauss I noticed you 👎 my comment, is there a specific part you'd like to be changed?

It's also worth noting the 'missing' v3.12 is specificly for the android compilation.

anonrig · 2024-08-14T02:26:06Z

Did I stutter?

@cclauss What's the basis of blocking this pull request? I didn't understand.

jasnell · 2024-08-14T02:53:11Z

@cclauss ... The "did I stutter" comment here is unproductive, unfriendly, and unhelpful. If you are requesting changes to this PR please articulate what those are.

cclauss · 2024-08-14T03:19:55Z

If you are requesting changes to this PR please articulate what those are.

My requested changes have already been stated. If you choose to ignore them then please proceed. My contributions to Node.js have been all about keeping its Python infrastructure current. My suggestions above are merely a continuation of that effort to ensure that Node.js users open fewer Python-related issues.

jasnell · 2024-08-14T03:27:08Z

I understand that you have requested changes. The issue is more the rather disrespectful way you presented your change request. The "did I stutter" comment was disrespectful.

Trott

LGTM with or without changing android-configure to support 3.12 (and 3.13, just to be synchronized with configure) but I do have a slight preference that it be changed in this PR.

cclauss

OK. I or someone else will revisit these files to complete the task.

Ignoring my review comments, marking them resolved, and leaving the work for others to do is disrespectful thus my snarkiness.

BUILDING.md

android-configure

Co-authored-by: Christian Clauss <[email protected]>

RedYetiDev · 2024-08-14T11:27:06Z

Ignoring my review comments, marking them resolved, and leaving the work for others to do is disrespectful thus my snarkiness.

I'm sorry if you feel my response was disrespectful. I had no intention of it coming off that way.

Ignoring my review comments

I don't want you to think I'm ignoring your review comments. Your insight is extremely important.

marking them resolved

I only marked one of your reviews as resolved, as I felt it would be easier to manage if there was one conversation rather than two going on about the same topic.

and leaving the work for others to do

I'm happy to do the work required for android's configure script to support 3.12 and 3.13, I just don't believe that work should be done in this PR.

cclauss · 2024-08-14T12:06:45Z

I think we are all good @RedYetiDev. I do appreciate your work and I apologize for being snarky.

I approved this PR so we can move forward...

nodejs-github-bot · 2024-08-23T19:10:58Z

Commit Queue failed

- Loading data for nodejs/node/pull/54358
✔  Done loading data for nodejs/node/pull/54358
----------------------------------- PR info ------------------------------------
Title      build: update required python version to 3.8 (#54358)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     RedYetiDev:python3.8 -> nodejs:main
Labels     doc, build, python, needs-ci, commit-queue-squash, dont-land-on-v18.x, dont-land-on-v20.x
Commits    5
 - build: update required python version to 3.8
 - Update pyproject.toml
 - Update BUILDING.md
 - Update BUILDING.md
 - Update BUILDING.md
Committers 2
 - RedYetiDev <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54358
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54358
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - Update BUILDING.md
   ⚠  - Update BUILDING.md
   ℹ  This PR was created on Tue, 13 Aug 2024 16:27:21 GMT
   ✔  Approvals: 3
   ✔  - Christian Clauss (@cclauss): https://github.com/nodejs/node/pull/54358#pullrequestreview-2237132795
   ✔  - Rich Trott (@Trott): https://github.com/nodejs/node/pull/54358#pullrequestreview-2237096436
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/54358#pullrequestreview-2237668891
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-19T21:13:51Z: https://ci.nodejs.org/job/node-test-pull-request/61250/
- Querying data for job/node-test-pull-request/61250/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10531055982

cclauss · 2024-08-23T21:16:23Z

@RedYetiDev the red message below says Merging is blocked.

Trott · 2024-08-23T21:17:29Z

@RedYetiDev the red message below says Merging is blocked.

I think that's what it says on all of our PRs. I think of it as a way to mostly make sure no one accidentally uses the GitHub button.

Trott · 2024-08-23T21:18:03Z

@Trott did you mean to remove all the other labels?

Nope. That's why I immediately re-added them. Not sure how they all got de-selected. I might need my web-browsing license revoked.

RedYetiDev · 2024-08-23T21:18:28Z

Nope. That's why I immediately re-added them. Not sure how they all got de-selected. I might need my web-browsing license revoked.

Happens to me more often that i can count... 😆

nodejs-github-bot · 2024-08-23T21:20:33Z

Landed in 22daeba

RedYetiDev · 2024-08-23T21:24:02Z

android-configure Python 3.13 support in a follow-up at #54529

Co-Authored-By: Jim B <[email protected]> PR-URL: #54358 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

build: update required python version to 3.8

ec66d4c

Co-Authored-By: Jim B <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. labels Aug 13, 2024

RedYetiDev added the python PRs and issues that require attention from people who are familiar with Python. label Aug 13, 2024

Update pyproject.toml

02097e9

RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 13, 2024

richardlau added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Aug 13, 2024

cclauss reviewed Aug 13, 2024

View reviewed changes

android-configure Show resolved Hide resolved

cclauss reviewed Aug 13, 2024

View reviewed changes

android-configure Show resolved Hide resolved

cclauss requested changes Aug 13, 2024

View reviewed changes

Update BUILDING.md

e6b47dd

RedYetiDev requested a review from cclauss August 13, 2024 20:10

RedYetiDev mentioned this pull request Aug 13, 2024

Update required python version #54112

Closed

Trott approved these changes Aug 14, 2024

View reviewed changes

cclauss approved these changes Aug 14, 2024

View reviewed changes

BUILDING.md Outdated Show resolved Hide resolved

BUILDING.md Outdated Show resolved Hide resolved

android-configure Show resolved Hide resolved

android-configure Show resolved Hide resolved

lpinca approved these changes Aug 14, 2024

View reviewed changes

RedYetiDev and others added 2 commits August 14, 2024 07:13

Update BUILDING.md

3ff39a3

Co-authored-by: Christian Clauss <[email protected]>

Update BUILDING.md

e7f8549

Co-authored-by: Christian Clauss <[email protected]>

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 23, 2024

Trott approved these changes Aug 23, 2024

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 23, 2024

nodejs-github-bot merged commit 22daeba into nodejs:main Aug 23, 2024
77 checks passed

RedYetiDev mentioned this pull request Aug 23, 2024

build: support up to python 3.13 in android-configure #54529

Open

RafaelGSS mentioned this pull request Aug 25, 2024

v22.8.0 proposal #54560

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: update required python version to 3.8 #54358

build: update required python version to 3.8 #54358

RedYetiDev commented Aug 13, 2024

RedYetiDev commented Aug 13, 2024 •

edited

Loading

cclauss commented Aug 13, 2024

cclauss left a comment •

edited

Loading

RedYetiDev commented Aug 13, 2024

RedYetiDev commented Aug 13, 2024 •

edited

Loading

anonrig commented Aug 14, 2024

jasnell commented Aug 14, 2024

cclauss commented Aug 14, 2024 •

edited

Loading

jasnell commented Aug 14, 2024

Trott left a comment

cclauss left a comment •

edited

Loading

RedYetiDev commented Aug 14, 2024 •

edited

Loading

cclauss commented Aug 14, 2024

nodejs-github-bot commented Aug 23, 2024

cclauss commented Aug 23, 2024

Trott commented Aug 23, 2024

Trott commented Aug 23, 2024

RedYetiDev commented Aug 23, 2024

nodejs-github-bot commented Aug 23, 2024

RedYetiDev commented Aug 23, 2024

build: update required python version to 3.8 #54358

build: update required python version to 3.8 #54358

Conversation

RedYetiDev commented Aug 13, 2024

RedYetiDev commented Aug 13, 2024 • edited Loading

cclauss commented Aug 13, 2024

cclauss left a comment • edited Loading

Choose a reason for hiding this comment

RedYetiDev commented Aug 13, 2024

RedYetiDev commented Aug 13, 2024 • edited Loading

anonrig commented Aug 14, 2024

jasnell commented Aug 14, 2024

cclauss commented Aug 14, 2024 • edited Loading

jasnell commented Aug 14, 2024

Trott left a comment

Choose a reason for hiding this comment

cclauss left a comment • edited Loading

Choose a reason for hiding this comment

RedYetiDev commented Aug 14, 2024 • edited Loading

cclauss commented Aug 14, 2024

nodejs-github-bot commented Aug 23, 2024

cclauss commented Aug 23, 2024

Trott commented Aug 23, 2024

Trott commented Aug 23, 2024

RedYetiDev commented Aug 23, 2024

nodejs-github-bot commented Aug 23, 2024

RedYetiDev commented Aug 23, 2024

RedYetiDev commented Aug 13, 2024 •

edited

Loading

cclauss left a comment •

edited

Loading

RedYetiDev commented Aug 13, 2024 •

edited

Loading

cclauss commented Aug 14, 2024 •

edited

Loading

cclauss left a comment •

edited

Loading

RedYetiDev commented Aug 14, 2024 •

edited

Loading