test: Do not write Python bytecode to source directory #30533

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:240726-rpcauth-ro changing 2 files +2 −1
  1. hebasto commented at 4:48 pm on July 26, 2024: member

    This change prevents writing share/rpcauth/__pycache__/*.pyc, which is especially useful for out-of-source builds when the source directory is supposed to be read-only.

    Also see #19385 (comment).

    I believe that performance concerns can be disregarded in this case.

  2. DrahtBot commented at 4:48 pm on July 26, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK stickies-v
    Stale ACK tdb3, BrandonOdiwuor

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on Jul 26, 2024
  4. tdb3 approved
  5. tdb3 commented at 11:42 pm on July 26, 2024: contributor

    ACK bce63879fe64ac32b7d3324757cf6e309a0edf9d Tested on master (share/rpcauth/__pycache__ was generated when running rpcauth-test.py) and on the PR branch (share/rpcauth/__pycache__ was not generated).

    Maybe it’s doing this for contrib/devtools/test-security-check.py and test-symbol-check.py as well?

  6. BrandonOdiwuor commented at 3:24 pm on July 30, 2024: contributor

    Tested ACK bce63879fe64ac32b7d3324757cf6e309a0edf9d

    Screenshot from 2024-07-30 18-23-24

  7. DrahtBot added the label CI failed on Aug 4, 2024
  8. DrahtBot removed the label CI failed on Aug 6, 2024
  9. hebasto commented at 9:43 am on August 7, 2024: member
    Friendly ping @stickies-v @maflcko :)
  10. maflcko commented at 10:02 am on August 7, 2024: member

    CI is green and the diff looks reasonable, so I guess it is harmless and fine.

    Did you try setting the source dir read-only, and it passes with this change?

  11. hebasto commented at 10:15 am on August 7, 2024: member

    Did you try setting the source dir read-only, and it passes with this change?

    Yes, it does. However, such a test is not very useful because Python just silently skips writing the bytecode cache if there are no write permissions.

    A more useful test can be conducted on a branch, which performs a full migration to CMake and cleans up all .gitignore files from Autotools remnants. Running ctest keeps the source tree pristine.

  12. in test/util/rpcauth-test.py:24 in bce63879fe outdated
    20@@ -21,6 +21,7 @@ def setUp(self):
    21         with open(config_path, encoding="utf8") as config_file:
    22             config.read_file(config_file)
    23         sys.path.insert(0, os.path.dirname(config['environment']['RPCAUTH']))
    24+        sys.dont_write_bytecode = True
    



    hebasto commented at 10:44 am on August 7, 2024:
    Thanks! Updated.
  13. stickies-v commented at 10:18 am on August 7, 2024: contributor

    Concept ACK, the performance benefits of caching bytecode are irrelevant here so keeping a clean source dir seems preferable.

    As an alternative approach, might it make more sense to use pycache_prefix so we can address this concern globally instead of just for this file, and in the build system code instead of leaking this into non-build system code? And that way we can keep the (in this case negligible) performance benefits of caching bytecode?

  14. hebasto commented at 10:37 am on August 7, 2024: member

    As an alternative approach, might it make more sense to use pycache_prefix so we can address this concern globally instead of just for this file, and in the build system code instead of leaking this into non-build system code? And that way we can keep the (in this case negligible) performance benefits of caching bytecode?

    I did consider it. I have to admit that I failed to find a solution that touches only the build system code and works in all scenarios, including running functional tests individually.

  15. test: Do not write Python bytecode to source directory
    This change prevents writing `share/rpcauth/__pycache__/*.pyc`, which is
    especially useful for out-of-source builds when the source directory is
    supposed to be read-only.
    c91a10b684
  16. hebasto force-pushed on Aug 7, 2024
  17. hebasto commented at 10:45 am on August 7, 2024: member
    Addressed @stickies-v’s #30533 (review).
  18. stickies-v commented at 11:17 am on August 7, 2024: contributor

    I failed to find a solution that touches only the build system code and works in all scenarios, including running functional tests individually.

    I’m also not sure that’s something we should be doing? Python programs creating pycache files is expected Python behaviour, and we can’t control if/how/when users run Python programs. Users who wish to do so can easily avoid this caching behaviour in a multitude of ways, the shortest of which is running python -B rpcauth-test.py or setting env vars. I think we should strive to make our build system behave as well as we can, and e.g. make check cluttering up the source dir seems like undesirable and not something the user can control, and thus we should address.

    In that, vein, I think my preferred approach for rpcauth-test.py would be to just update Makefile.test.include to something like this (but using some build dir instead of tmp):

     0diff --git a/src/Makefile.test.include b/src/Makefile.test.include
     1index 3d04498369..43c4d25ed7 100644
     2--- a/src/Makefile.test.include
     3+++ b/src/Makefile.test.include
     4@@ -438,7 +438,7 @@ if BUILD_BITCOIN_TX
     5 	$(PYTHON) $(top_builddir)/test/util/test_runner.py
     6 endif	[@echo](/bitcoin-bitcoin/contributor/echo/) "Running test/util/rpcauth-test.py..."
     7-	$(PYTHON) $(top_builddir)/test/util/rpcauth-test.py
     8+	$(PYTHON) -X pycache_prefix=/tmp/pycache/ $(top_builddir)/test/util/rpcauth-test.py
     9 if TARGET_WINDOWS
    10 else
    11 if ENABLE_BENCH
    12diff --git a/test/util/rpcauth-test.py b/test/util/rpcauth-test.py
    13index 931eb80960..8a7ff26dcb 100755
    14--- a/test/util/rpcauth-test.py
    15+++ b/test/util/rpcauth-test.py
    16@@ -21,7 +21,6 @@ class TestRPCAuth(unittest.TestCase):
    17         with open(config_path, encoding="utf8") as config_file:
    18             config.read_file(config_file)
    19         sys.path.insert(0, os.path.dirname(config['environment']['RPCAUTH']))
    20-        sys.dont_write_bytecode = True
    21         self.rpcauth = importlib.import_module('rpcauth')
    22 
    23     def test_generate_salt(self):
    

    All that said, this is a small change and I don’t want to block progress. It’s a small and reasonable diff, and easily reverted/changed in the future. I suppose it’s more of a philosophical concern.

  19. hebasto commented at 1:13 pm on August 15, 2024: member
    Closing until the migration to CMake is complete.
  20. hebasto closed this on Aug 15, 2024

  21. vasild commented at 9:56 am on August 16, 2024: contributor

    ACK c91a10b6843b9c48cb200592e025bd2a0fcb8a7e in case this is reconsidered.

    IMO this approach is better than leaving it as it is.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-18 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me