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

warnings.filters was modified by test_check_c_globals #95349

Closed
zooba opened this issue Jul 27, 2022 · 17 comments
Closed

warnings.filters was modified by test_check_c_globals #95349

zooba opened this issue Jul 27, 2022 · 17 comments
Labels
tests Tests in the Lib/test dir

Comments

@zooba
Copy link
Member

zooba commented Jul 27, 2022

When I run my own automated tests against main, I get the following test failure on macOS and Ubuntu.

Warning -- warnings.filters was modified by test_check_c_globals
Warning --   Before: (4544233792, [('error', None, <class 'BytesWarning'>, None, 0), ('default', None, <class 'Warning'>, None, 0), ('default', None, <class 'DeprecationWarning'>, '__main__', 0), ('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0)], [('error', None, <class 'BytesWarning'>, None, 0), ('default', None, <class 'Warning'>, None, 0), ('default', None, <class 'DeprecationWarning'>, '__main__', 0), ('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0)])
Warning --   After:  (4544233792, [('error', None, <class 'BytesWarning'>, None, 0), ('default', None, <class 'Warning'>, None, 0), ('default', None, <class 'DeprecationWarning'>, '__main__', 0), ('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0)], [('error', None, <class 'BytesWarning'>, None, 0), ('default', None, <class 'Warning'>, None, 0), ('default', None, <class 'DeprecationWarning'>, '__main__', 0), ('ignore', None, <class 'DeprecationWarning'>, None, 0), ('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), ('ignore', None, <class 'ImportWarning'>, None, 0), ('ignore', None, <class 'ResourceWarning'>, None, 0), ('ignore', None, <class 'pkg_resources.PEP440Warning'>, None, 0)])

We've previously added code to preserve warnings.filters around tests that import pkg_resources it seems. I'm not entirely sure where it's coming from in this case, as it doesn't repro on Windows (or apparently on our buildbots), but I guess we need the same preservation around it.

@ericsnowcurrently I think this may be yours?

@zooba zooba added the tests Tests in the Lib/test dir label Jul 27, 2022
@ericsnowcurrently
Copy link
Member

I'll take a look.

@zooba
Copy link
Member Author

zooba commented Jul 28, 2022

I patched my own copy to have this as the test:

    def test_check_c_globals(self):
        with save_restore_warnings_filters():
            with test.test_tools.imports_under_tool('c-analyzer'):
                from cpython.__main__ import main
            try:
                main('check', {})
            except NotImplementedError:
                raise unittest.SkipTest('not supported on this host')

Solves the changed global, but I only assume the test is still valid.

@ericsnowcurrently
Copy link
Member

Sorry for the delay. I'll try to take a look today.

@ericsnowcurrently
Copy link
Member

Hmm, that test is disabled and the code doesn't look like yours. Are you based on some other branch?

https://github.com/python/cpython/blob/main/Lib/test/test_check_c_globals.py#L14-L19

@zooba
Copy link
Member Author

zooba commented Aug 1, 2022

Yeah, I didn't merge any changes. It's just in my internal fork.

The test may be disabled, but the import still happens, and that's apparently what leaks the change.

@ericsnowcurrently
Copy link
Member

I'm unclear on why test_check_c_globals would be causing this problem. Neither it nor anything in Tools/c-analyzer messes with warnings. Perhaps it is something with test_tools? (I'm using a couple helpers from there.)

Is this a new failure?

FWIW, I don't get a failure when I run the test locally. How are you invoking the tests?

@zooba
Copy link
Member Author

zooba commented Aug 1, 2022

I think it's something in the c-analyzer tool triggering it. Considering the warning being filtered is from pkgutil, it might be loading distutils/setuptools or pip? But once I remove the import from the test module, it doesn't trigger anymore.

Maybe you've got something else in your environment that loads this stuff earlier? So then it doesn't look like the test is adding the extra warning filter because it's already there?

I'm basically just running normal CI, it's just not on GitHub Actions.

@zooba
Copy link
Member Author

zooba commented Aug 1, 2022

No, it's not quite normal. I configure with custom --prefix and make install before doing make buildbottest, so I'm not sure how different that makes things.

@ericsnowcurrently
Copy link
Member

I think it's something in the c-analyzer tool triggering it. Considering the warning being filtered is from pkgutil, it might be loading distutils/setuptools or pip? But once I remove the import from the test module, it doesn't trigger anymore.

Yeah, I suspect this is a consequence of PEP 632 and the fact that we're still using parts of distutils in the c-analyzer (or at least were until 11 days ago).

Do you still see the problem with the latest main?

Regardless, I don't mind adding the fix you suggested if needed.

Maybe you've got something else in your environment that loads this stuff earlier? So then it doesn't look like the test is adding the extra warning filter because it's already there?

I bet that's it.

@zooba
Copy link
Member Author

zooba commented Aug 3, 2022

Do you still see the problem with the latest main?

My build from 7 hours ago failed in the same way. I wouldn't have thought it was coming from distutils though, unless it's coming from the distutils test suite?

@ericsnowcurrently
Copy link
Member

FYI, here's what I see:

$ ./python -m test test_check_c_globals
0:00:00 load avg: 0.15 Run tests sequentially
0:00:00 load avg: 0.15 [1/1] test_check_c_globals
$CPYTHON_ROOOT/Tools/c-analyzer/c_parser/preprocessor/common.py:2: DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives
  import distutils.ccompiler

== Tests result: SUCCESS ==

1 test OK.

Total duration: 278 ms
Tests result: SUCCESS

I do get the warning but it doesn't cause a failure.

@ericsnowcurrently
Copy link
Member

I was able to reproduce the failure you originally described with an out-of-tree build and a custom --prefix:

mkdir ../build
cd ../build
../cpython/configure --cache-file=../python-config.cache --enable-ipv6 --prefix=$HOME/build/installed CFLAGS=-O0 --with-system-expat --with-system-ffi --with-pydebug
make -j8
make install
./python -E ../cpython/Tools/scripts/run_tests.py -j 1 -u all -W --slowest --fail-env-changed --timeout=1200  test_check_c_globals

@ericsnowcurrently
Copy link
Member

There's a comment in the distutils test that explains the situation:

# bpo-40055: Save/restore warnings filters to leave them unchanged.
# Importing tests imports docutils which imports pkg_resources
# which adds a warnings filter.
with save_restore_warnings_filters():
__import__(modname)

@ericsnowcurrently
Copy link
Member

The referenced issue, gh-84236 (AKA bpo-40055) is basically the same as this one. Looks like here it's only happening with the specific build steps.

@ericsnowcurrently
Copy link
Member

My fix (gh-95837) should be good enough until we stop using distutils in the c-analyzer.

miss-islington pushed a commit that referenced this issue Aug 9, 2022
…H-95837)

Under certain build conditions, test_check_c_globals fails.  This fix takes the same approach as we took for gh-84236 (via gh-20095).  We'll be removing use of distutils in the c-analyzer at some point.  Until then we'll hide the warning filter.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 9, 2022
…als (pythonGH-95837)

Under certain build conditions, test_check_c_globals fails.  This fix takes the same approach as we took for pythongh-84236 (via pythongh-20095).  We'll be removing use of distutils in the c-analyzer at some point.  Until then we'll hide the warning filter.
(cherry picked from commit 3ff6d9a)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ambv pushed a commit that referenced this issue Aug 10, 2022
…H-95837) (GH-95843)

Under certain build conditions, test_check_c_globals fails.  This fix takes the same approach as we took for gh-84236 (via gh-20095).  We'll be removing use of distutils in the c-analyzer at some point.  Until then we'll hide the warning filter.
(cherry picked from commit 3ff6d9a)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@ericsnowcurrently
Copy link
Member

Thanks for pointing this out, @zooba. It should be all fixed now. Would you mind verifying?

@zooba
Copy link
Member Author

zooba commented Aug 10, 2022

It looks good in my build overnight 👍

@zooba zooba closed this as completed Aug 10, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 10, 2022
…als (pythonGH-95837)

Under certain build conditions, test_check_c_globals fails.  This fix takes the same approach as we took for pythongh-84236 (via pythongh-20095).  We'll be removing use of distutils in the c-analyzer at some point.  Until then we'll hide the warning filter.
(cherry picked from commit 3ff6d9a)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
miss-islington added a commit that referenced this issue Aug 11, 2022
…H-95837)

Under certain build conditions, test_check_c_globals fails.  This fix takes the same approach as we took for gh-84236 (via gh-20095).  We'll be removing use of distutils in the c-analyzer at some point.  Until then we'll hide the warning filter.
(cherry picked from commit 3ff6d9a)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
iritkatriel pushed a commit to iritkatriel/cpython that referenced this issue Aug 11, 2022
…als (pythonGH-95837)

Under certain build conditions, test_check_c_globals fails.  This fix takes the same approach as we took for pythongh-84236 (via pythongh-20095).  We'll be removing use of distutils in the c-analyzer at some point.  Until then we'll hide the warning filter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

No branches or pull requests

2 participants