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

Fix handling of C standard support for Emscripten #13221

Merged
merged 1 commit into from
May 19, 2024

Conversation

rgommers
Copy link
Contributor

Emscripten version numbers are unrelated to Clang version numbers, so it is necessary to change the version checks for c_std=c17 & co. Without that, no project that defaults to C17 or newer will build with Emscripten.

See pyodide/pyodide#4762 for more context. Also note that this bug caused defaulting to C17 in scikit-learn to be reverted (scikit-learn#29015), and it may be a problem for SciPy 1.14.0 too since that release will upgrade from C99 to C17.

Cc @lesteve.

Emscripten version numbers are unrelated to Clang version numbers,
so it is necessary to change the version checks for `c_std=c17` & co.
Without that, no project that defaults to C17 or newer will build with
Emscripten.

See pyodide/pyodide#4762 for more
context. Also note that this bug caused defaulting to C17 in
scikit-learn to be reverted (scikit-learn#29015), and it may be a
problem for SciPy 1.14.0 too since that release will upgrade from C99
to C17.

Co-authored-by: Loic Esteve <loic.esteve@ymail.com>
@eli-schwartz
Copy link
Member

Neither CI failure seems relevant. Pylint updated and seems to be reporting a bunch of false positives, rust fails to link on Windows due to I can only assume a rustc update.

@rgommers
Copy link
Contributor Author

Neither CI failure seems relevant.

Do I need to rebase once those are resolved on master, or is this good as is?

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

Changes seem correct. The CI failures can be easily checked as not related.

@eli-schwartz eli-schwartz merged commit 77db04f into mesonbuild:master May 19, 2024
30 of 33 checks passed
@rgommers rgommers deleted the fix-emscripten-cstd branch May 19, 2024 15:47
@rgommers
Copy link
Contributor Author

Thanks for the reviews. Could this possibly be included in 1.4.1? I'm not sure how the backport process works, so I thought I'd ask here.

@eli-schwartz
Copy link
Member

eli-schwartz commented May 22, 2024

It's obviously a bug fix rather than a feature, so it should be eligible, yes.

The other concern really is just that if you create a build directory with the old meson, then backport the change and reuse the existing build directory, it should never crash due to breaking the pickle-based binary layout of the private coredata (for 1.5.0, we specifically error out and say to regenerate the build directory but bugfixes are drop-in compatible).

@rgommers
Copy link
Contributor Author

Makes sense. Since building with std=c17 didn't work at all with Emscripten and the behavior for older C standards is unchanged, I'd expect no change to the private coredata that is relevant.

If it's eligible, should I open a PR for the backport, or is that all done in one go?

@eli-schwartz
Copy link
Member

Changes to the private coredata would be e.g. if pickled code tried to evaluate nonexistent code, e.g. when building with older stds or loading unittests / installdata.

I don't think that should be a problem here, although double checking could still be good.

Either way if it's good to go, set the milestone and it will be backported automatically as part of the release process. Manual backports are only necessary if there are merge conflicts when cherry picking.

@tristan957 tristan957 added this to the 1.4.1 milestone May 22, 2024
@tristan957
Copy link
Contributor

Added to milestone. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants