Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Oct 3, 2024

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

These flags are only needed for node <= 16. Node 16 has been end of life since
October of 2023.
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
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

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 that 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
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.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants