GeezerVison

@hugovk FYI, --disable-safety configure option would disable the warnings locally, but maybe we should flip from opt-out to opt-in for these new warnings until we get to a place with fewer in the codebase?

See:

Concretely I'm wondering if we could instead of reverting the whole PR switch the option instead or something else to maintain the CI testing?

Or maybe we can maintain the list of files that currently have these warnings and exclude them. It will allow us to fix these warnings in files one by one.

@sobolevn We have that already, but for determining whether to fail CI. The warnings still occur locally, but as you've seen there is quite a few of them. I'm not sure how possible filtering them from output would be @nohlson?

@hugovk I think it's also okay to revert the whole PR and then squash everything and re-add w/ the proper opt-in configure option instead of opt-out. Don't want to be too disruptive to core devs if you think any of the above suggestions might take too long.

@hugovk @sethmlarson This is a PR that makes the "safety" options opt-in to allow the CI warning checking to remain: #124070 . @hugovk Maybe this could be a suitable alternative to reverting #123020

Thanks @nohlson for the quick response!

@colesbury Hmm, then let's revert.

We can then update #124070 to "un-revert" and also fix the CI failure (or create a fresh PR, whichever is easier).

See:

• python/cpython/actions/runs/10854542283/job/30125214880?pr=124069
• python/cpython/actions/runs/10854746319/job/30125872824?pr=124070

Correcting myself: that sort of thing is expected, those PRs introduced new warnings. The devguide link in the CI failure tells what to do: ideally fix your new warning, or add it to the ignore list if that's not possible.

Correcting myself again! Those look like the failure in #124070, and needs fixing. This sort of thing is expected. Now I'll close my laptop for the evening :)

I think reverting first is the right approach. Currently the "Ubuntu / build and test" CI is failing on "Check compiler warnings" on new PRs.

See:

• https://github.com/python/cpython/actions/runs/10854542283/job/30125214880?pr=124069
• https://github.com/python/cpython/actions/runs/10854746319/job/30125872824?pr=124070

In #124069 the tool is failing to parse the compiler output because it looks mangled

../cpython-ro-srcdir/Modules/_hacl/Hacl_Hash_Blake2s_Simd128.c:108:67: warning: conversion to ‘int’ from ‘uint32_t’ {aka ‘unsigned int’} may change theIn file included from ../cpython-ro-srcdir/Modules/_hacl/include/krml/FStar_UInt128_Verified.h:10,

And that is the same for #124070

../cpython-ro-srcdir/Modules/expat/xmlparse.c:3101:46: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-Modules/_hacl/Hacl_Hash_Blake2s_Simd128.c:80:3: note: in expansion of macro ‘KRML_MAYBE_FOR10’

This might be the cmake jobs racing to write to the log so it might just need cmake option --output-sync

I also agree on reverting, because the very same PR that is alternative to this one (#124070) has lots of warnings.

The warnings will still be generated in CI for the Ubuntu build and test and macOS build and test jobs.

Here's take two: #124070