This is an archive of the discontinued LLVM Phabricator instance.

[gn build] Fix Python DeprecationWarning
ClosedPublic

Authored by mantognini on Sep 19 2019, 2:41 AM.

Details

Summary

This fixes two issues:

  • DeprecationWarning: invalid escape sequence \`
  • ResourceWarning: unclosed file

Event Timeline

mantognini created this revision.Sep 19 2019, 2:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2019, 2:41 AM

Let me know what you think of this patch. Thanks.

thakis added inline comments.Sep 19 2019, 12:49 PM
llvm/utils/gn/build/write_cmake_config.py
65

What warns about this? Python itself?

The garbage collector cleans up the file, and this is a very short-lived script anyways, so I'd prefer brevity over linter happyiness if this is to placate some linter.

If python3 warns about this by default, I suppose it's fine to fix this.

mantognini marked an inline comment as done.Sep 24 2019, 3:34 AM
mantognini added inline comments.
llvm/utils/gn/build/write_cmake_config.py
65

Yep, starting with Python 3.7.4 and PEP565, those warnings are emitted by default:

To improve this situation, this PEP proposes a single adjustment to the default warnings filter: displaying deprecation warnings attributed to the main module by default.

This is not an extra linter that warns about this, but Python itself.

I'd further recommend fixing this to reduce the noise in our downstream tests.

thakis accepted this revision.Sep 24 2019, 5:28 AM
This revision is now accepted and ready to land.Sep 24 2019, 5:28 AM
thakis requested changes to this revision.Sep 24 2019, 5:53 AM

Hm, I'm trying to reproduce the warnings locally by adding script_executable = "python3" to llvm/utils/gn/.gn and doing a full build. Everything looks happy. I checked that out/gn/toolchain.ninja does indeed invoke python3 for python actions. What am I doing wrong? How do I reproduce these warnings?

This revision now requires changes to proceed.Sep 24 2019, 5:53 AM

You need Python 3.7.4 or later. The warnings are emitted only starting from this minor release.

I'm on exactly 3.7.4:

$ python3 --version
Python 3.7.4

Can you paste the commands you run locally to see the warnings?

I'm afraid I can't give you the exact log we have, because we have a special integration of LLVM downstream and don't directly rely on the usual cmake/gn/... workflow. I'm sorry about that.

What I can tell you is that we invoke this script as a standalone program, i.e. we run ./some/path/write_cmake_config.py <args>. This means the she-bang line is being used, and python (not python3) is being used. I don't know exactly how this script is invoked when using the "regular" build-flow, but could it be that the python binary is actually Python 2?

What's the output of /usr/bin/env python --version on your system?

/usr/bin/env python --version
Python 3.7.0

Apparently I was slightly wrong: it's not specific to the .4 maintenance release but any 3.7 version. On the download page I though the items applied to 3.7.4 only, but as can be confirmed by the 3.7 documentation page it's not. Apologies for this. The fix is still relevant though.

I assume you couldn't reproduce the warnings. Do you have by any change some PYTHONWARNINGS environment variable turning the warning off or some command line argument that does the same?

thakis accepted this revision.Sep 24 2019, 8:07 AM

Apparently I was slightly wrong: it's not specific to the .4 maintenance release but any 3.7 version. On the download page I though the items applied to 3.7.4 only, but as can be confirmed by the 3.7 documentation page it's not. Apologies for this. The fix is still relevant though.

I assume you couldn't reproduce the warnings. Do you have by any change some PYTHONWARNINGS environment variable turning the warning off or some command line argument that does the same?

Aha, thanks, if I run PYTHONWARNINGS=default ninja -C out/gn -j200 check-clang I see the warnings. But by default they're not on for me, and I don't have PYTHONWARNINGS set. Do you set it to anything? Does anything else set it in your env?

This revision is now accepted and ready to land.Sep 24 2019, 8:07 AM

I don't have PYTHONWARNINGS set in my env. Maybe ninja invoke the script using python -Wignore write_cmake_config.py or something? It's quite worrisome that Python runtime can exhibit different behaviours without apparent reasons. :/

I don't have PYTHONWARNINGS set in my env. Maybe ninja invoke the script using python -Wignore write_cmake_config.py or something? It's quite worrisome that Python runtime can exhibit different behaviours without apparent reasons. :/

That's mysterious indeed. Looks like the default warnings come from here: https://github.com/python/cpython/blob/88e6447451fb5525e83e802c66c3e51b4a45bf86/Python/initconfig.c#L2064 You don't happen to run python in dev mode, do you? In dev mode, the warnings default to "default"; else they apparently don't.

That's mysterious indeed. Looks like the default warnings come from here: https://github.com/python/cpython/blob/88e6447451fb5525e83e802c66c3e51b4a45bf86/Python/initconfig.c#L2064 You don't happen to run python in dev mode, do you? In dev mode, the warnings default to "default"; else they apparently don't.

I think I found out why I get those warnings: the Python binary we use for our test was built with --with-pydebug. I've tested this by downloading Python source and building it with and without that ./configure option and it seems to change the behaviour of Python runtime. Using GDB, I inspected config_init_warnoptions. The options are set apparently in the same way. Turns out, the difference are in init_filters, which turns all warnings on when built using --with-pydebug but only the ones mentioned in the documentation when built without this option, and in get_filter, which uses get_default_action under --with-pydebug.

Strictly speaking, this means that the warnings we see downstream are not triggered by default with Python 3.7. I still think it's worth fixing these, though.

This revision was automatically updated to reflect the committed changes.

Strictly speaking, this means that the warnings we see downstream are not triggered by default with Python 3.7. I still think it's worth fixing these, though.

Meh. The backslash escape fix maybe, but the files are freed at script end anyways, so it's a bunch of lines more for close to 0 benefit ¯\_(ツ)_/¯

…but thanks for getting to the bottom of why you're seeing the warnings, that's good to know :)

And given that this is committed already, it's fine to keep it in.