8000 gh-124928: Emscripten node support: Clean up old node <= 16 flags by hoodmane · Pull Request #124929 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-124928: Emscripten node support: Clean up old node <= 16 flags #124929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 29, 2024
Merged

gh-124928: Emscripten node support: Clean up old node <= 16 flags #124929

merged 4 commits into from
Oct 29, 2024

Conversation

hoodmane
Copy link
Contributor
@hoodmane hoodmane commented Oct 3, 2024 •
edited
Loading

These flags are only needed for node <= 16. Node 16 has been end of life since
October of 2023.

@hoodmane
Emscripten: Clean up flags for old node versions
f90c237 
These flags are only needed for node <= 16. Node 16 has been end of life since
October of 2023.
@bedevere-app bedevere-app bot mentioned this pull request Oct 3, 2024
Closed
corona10
corona10 reviewed Oct 3, 2024
View reviewed changes
Copy link
Member
@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Hmm, can we add some assertion to check that user is using Node > 16?

@hoodmane
Copy link
Contributor Author
hoodmane commented Oct 3, 2024

Yeah that makes sense I will do that.

@hoodmane hoodmane requested a review from brettcannon as a code owner October 3, 2024 13:16
@hoodmane
Copy link
Contributor Author
hoodmane commented Oct 3, 2024

Okay now it prints the following complaint to standard error and exits with code 1 on old node versions.

Node version must be >= 18, got version v16.20.2

corona10
corona10 requested changes Oct 3, 2024
View reviewed changes
Copy link
Member
@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Please also add NEWS.d that now CPython requires Node >= 18 for Emscripten build :)

@bedevere-app
Copy link
bedevere-app bot commented Oct 3, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review t 8000 hat you're ready for them to take another look at this pull request.

@brettcannon brettcannon requested review from freakboy3742 and removed request for brettcannon October 3, 2024 17:39
willingc
willingc approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor
@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks @hoodmane for cleaning up the old node stuff. Much appreciated.

freakboy3742
freakboy3742 requested changes Oct 3, 2024
View reviewed changes
Copy link
Contributor
@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Code changes look good; as noted elsewhere, a news entry is definitely required (the devguide has details on how to generate one).

I'll also hold off merging until python/steering-council#256 is resolved.

@bedevere-app
Copy link
bedevere-app bot commented Oct 3, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@hoodmane hoodmane force-pushed the emscripten-node-flag-cleanup branch from c3360eb to 83d5082 Compare October 4, 2024 15:29
@hoodmane hoodmane force-pushed the emscripten-node-flag-cleanup branch from 83d5082 to 96e9e40 Compare October 4, 2024 15:31
corona10
corona10 approved these changes Oct 5, 2024
View reviewed changes
Copy link
Member
@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm!

@hoodmane
Copy link
Contributor Author

@freakboy3742 can we merge now that python/steering-council#256 is approved?

freakboy3742
freakboy3742 approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor
@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

LGTM!

@freakboy3742 freakboy3742 enabled auto-merge (squash) October 29, 2024 05:50
@freakboy3742 freakboy3742 merged commit dc2552d into python:main Oct 29, 2024
47 checks passed
@hoodmane hoodmane deleted the emscripten-node-flag-cleanup branch October 29, 2024 08:39
picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
@hoodmane @picnixz
pythongh-124928: Emscripten node support: Clean up old node <= 16 fla…
ddf800c 
…gs (python#124929)

Clean up configure flags for old node versions

These flags are only needed for node <= 16. Node 16 has been end of life since
October of 2023.
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@hoodmane @ebonnal
pythongh-124928: Emscripten node support: Clean up old node <= 16 fla…
6a08e52 
…gs (python#124929)

Clean up configure flags for old node versions

These flags are only needed for node <= 16. Node 16 has been end of life since
October of 2023.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freakboy3742 freakboy3742 freakboy3742 approved these changes

@willingc willingc willingc approved these changes

@corona10 corona10 corona10 approved these changes

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

Assignees

@freakboy3742 freakboy3742

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hoodmane @freakboy3742 @willingc @corona10
0