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-96398: Purge Emscripten code from configure.ac #117836

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Apr 13, 2024

@erlend-aasland

This comment was marked as outdated.

@erlend-aasland erlend-aasland enabled auto-merge (squash) April 13, 2024 07:33
@erlend-aasland erlend-aasland disabled auto-merge April 13, 2024 07:33
@erlend-aasland
Copy link
Contributor Author

cc. @corona10

@corona10
Copy link
Member

What about this change? #117819

@erlend-aasland
Copy link
Contributor Author

What about this change? #117819

IMO that should remain. We want to filter emcc out in ac_cv_cc_name switches.

@corona10
Copy link
Member

corona10 commented Apr 13, 2024

#if defined(__EMSCRIPTEN__)
  emcc

But this code is no reason to remain if we purge them no?

@erlend-aasland
Copy link
Contributor Author

#if defined(__EMSCRIPTEN__)
  emcc

But this code is no reason to remain if we purge them no?

emcc also defines __clang__ and __GNUC__ :(

@corona10
Copy link
Member

corona10 commented Apr 13, 2024

Okay, so to filter out emcc, we need emcc :(

Would you like to add a comment on that code to annotate that we do not support each according to PEP?

@erlend-aasland
Copy link
Contributor Author

Okay, so to filter out emcc, we need emcc :(

Or to put it another way: to filter out emcc, we need to know how to identify it :)

Would you like to add a comment on that code to annotate that we do not support each according to PEP?

We already have the switch in L1130. Did you think about a comment in the check that defines ac_cv_cc_name?

@erlend-aasland
Copy link
Contributor Author

Alternatively, we can just keep this code; there is not a huge maintenance burden; a solution could be to add a comment above the lines that are proposed to be purged by this PR.

@vstinner
Copy link
Member

I don't get why this code has to be removed right now. CPython is full of code specific to "unsupported" platforms such as MinGW or OpenBSD. There are CPython users using this build mode: build CPython for the browser.

@erlend-aasland
Copy link
Contributor Author

I don't get why this code has to be removed right now. CPython is full of code specific to "unsupported" platforms such as MinGW or OpenBSD. There are CPython users using this build mode: build CPython for the browser.

Yes, let's go with my suggestion from #117836 (comment).

@erlend-aasland erlend-aasland deleted the autoconf/really-drop-emscripten branch April 15, 2024 21:21
@vstinner
Copy link
Member

"CPython in the browser" is a top topic with https://pyodide.org/ and https://pyscript.net/ projects.

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.

3 participants