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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
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?
Tested ACK bce63879fe64ac32b7d3324757cf6e309a0edf9d
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?
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.
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
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?
As an alternative approach, might it make more sense to use
pycache_prefixso 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.
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.
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.
ACK c91a10b6843b9c48cb200592e025bd2a0fcb8a7e in case this is reconsidered.
IMO this approach is better than leaving it as it is.