Revert "gh-112301: Enable warning emitting options and ignore warnings in CI (#123020)"

[hugovk]

This reverts commit cfe6074 from #123020.

Fixes #124064.

There are too many warnings showing up when building locally.

We'll need to think of something else for #123020, perhaps only enable these when on the CI (for example, there's a CI=true environment variable).

• Issue: Consider applying flags for warnings about potential security issues #112301

[sethmlarson]

@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:

cpython/configure.ac

Line 2509 in 9f42b62

if test "$disable_safety" = "no"

[sethmlarson]

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?

[sobolevn]

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.

[sethmlarson]

@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?

[sethmlarson]

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

[nohlson]

@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

[colesbury]

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

[sobolevn]

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

[hugovk]

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

[hugovk]

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.

[hugovk]

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 :)

[nohlson]

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.

[hugovk]

Here's take two: #124070