BIP-0327: avoid using sys.path[0] to find current working directory#2158
BIP-0327: avoid using sys.path[0] to find current working directory#2158omipheo wants to merge 2 commits into
Conversation
`bip-0327/reference.py` and `bip-0327/gen_vectors_helper.py` both locate the test-vector JSON files via `os.path.join(sys.path[0], 'vectors', '<file>.json')`. As Sebastian Falbesoner noted in 4e18ee6 ("BIP-374: avoid using sys.path[0] to find current working directory"), this pattern is incompatible with any future `sys.path.insert(0, ...)` extension — for example, the `sys.path.insert(0, str(Path(__file__).parent / "secp256k1lab/src"))` shim that bip-0352 and bip-0374 use to locate their vendored `secp256k1lab` copies. Should bip-0327 follow the same vendoring pattern, every test-vector load would silently miss its file. Switch both files to the `Path(__file__).parent / 'vectors' / '...'` pattern so the path is always resolved relative to the script itself, matching the BIP-374 precedent. `import os` and `import sys` are removed from `reference.py` (no remaining uses); `gen_vectors_helper.py` gets an explicit `from pathlib import Path` since it can no longer inherit those names through `from reference import *`. Verified that `python3 reference.py` and `python3 gen_vectors_helper.py` still exit 0 with all 8 (resp. 2) vector files loading correctly. No behavior change beyond the path-resolution robustness.
jonatack
left a comment
There was a problem hiding this comment.
ACK, it's a minor refactor but more idiomatic and robust and would serve as a better reference for developers.
|
@omipheo note that BIPs 89, 328 and 340 also still use the same pattern. Maybe update them all here in one go. |
…rectory Same fix as the previous commit on BIP-0327, applied to the remaining reference implementations that still use the `os.path.join(sys.path[0], ...)` pattern: bip-0089/reference.py (8 sites), bip-0328/reference.py (1 site), and bip-0340/reference.py (1 site). All resolve test-vector paths via `Path(__file__).parent / ...` so future `sys.path.insert(0, ...)` extensions for vendored deps won't silently miss the data files. `import os` and `import sys` are dropped from each file (no remaining uses). bip-0340/test-vectors.py imports `sys` itself for `sys.stdout` so its wildcard `from reference import *` is unaffected by the import cleanup in bip-0340/reference.py. Verified: `python3 reference.py` for each of bip-0089, bip-0328, bip-0340 still exits 0 with all test vectors loading; bip-0340's test-vectors.py importer also still exits 0. Per @jonatack's review note on PR bitcoin#2158.
|
Done — pushed Verified each script still runs clean: Happy to squash both commits into one if you'd prefer a single atomic change before merge. |
murchandamus
left a comment
There was a problem hiding this comment.
I don’t see the point of this change. These python scripts accompany written specification documents as reference implementations, they are not living code. They are not expected to be updated, so they wouldn’t break. If someone were to add to these tests at a later time, and were to run into files not being loaded at that time, they could prompt an LLM to fix it as easily then.
Concept -0, I don’t think we should encourage this sort of code churn, but if you feel that it’s an improvement, please feel free to merge it, @jonatack.
Summary
bip-0327/reference.pyandbip-0327/gen_vectors_helper.pyboth locate the test-vector JSON files viaos.path.join(sys.path[0], 'vectors', '<file>.json'). As Sebastian Falbesoner noted in 4e18ee6 ("BIP-374: avoid using sys.path[0] to find current working directory"), this pattern is incompatible with any futuresys.path.insert(0, ...)extension — for example, thesys.path.insert(0, str(Path(__file__).parent / "secp256k1lab/src"))shim that bip-0352 and bip-0374 use to locate their vendoredsecp256k1labcopies. Should bip-0327 follow the same vendoring pattern, every test-vector load would silently miss its file.This PR switches both files to the
Path(__file__).parent / 'vectors' / '...'pattern so the path is always resolved relative to the script itself, matching the BIP-374 precedent.import osandimport sysare removed fromreference.py(no remaining uses);gen_vectors_helper.pygets an explicitfrom pathlib import Pathsince it can no longer inherit those names throughfrom reference import *.Diff
os.path.join(sys.path[0], 'vectors', 'X.json')Path(__file__).parent / 'vectors' / 'X.json'reference.py: 8 call sites (lines 499, 508, 532, 559, 577, 660, 717, 764).gen_vectors_helper.py: 2 call sites (lines 21, 49).Imports adjusted to match.
Verification
Both scripts continue to exit 0 with all test vectors loading.
(Note:
bash bip-0327/tests.shexits non-zero on master too — current local mypy versions flag four pre-existingbytearray/bytesmismatches atreference.py:354,355,671,672that are unrelated to this PR. CI is presumably pinned to an older mypy; the runtime tests themselves pass on both branches.)Notes
Same shape, same justification, applied to the next reference implementation in the same family.