Having the binary related utils sit in the test_framework.py is fine. However, they are mostly stand-alone utils, which may be used externally.
So move them to utils.py, to allow easier external use. The diff is trivial and can be reviewed via the git options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space.
Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33633.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | kevkevinpal, Sjors, janb84, musaHaruna, yuvicc, enirox001 |
| Concept ACK | l0rinc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
253 |
254 | def setup(self):
255 | """Call this method to start up the test framework object with options set."""
256 |
257 | check_json_precision()
258 | + export_env_build_path(self.config)
is there a reason why you moved this logic above this line of code?
self.options.cachedir = os.path.abspath(self.options.cachedir)
i don't think the order matters, because the two lines are completely unrelated and independent
No problem was just checking to see if there was a reason for it
ACK fa75ef4
Looks straightforward to me and makes sense to move those functions outside of the test framework to be used externally
lgtm ACK fa75ef4328f638221bcf85fcbefa885122084622
ACK fa75ef4328f638221bcf85fcbefa885122084622
Pr moves Helper class for information about bitcoin binaries to util.py.
To move the a helper class outside Test framework makes sense to me, this cleans up the test_framework.py code
Not completely convinced that these functions in the helper class will be used externally due to the tight coupling with these config settings config["environment"]["BUILDDIR"], bin", (this could be a good follow-up, if needed by external testers)
Code review ACK fa75ef4328f638221bcf85fcbefa885122084622
The Binaries class and get_binary_paths() are logically separate from TestFramework orchestration, so moving them to util.py makes sense for modularity.
As mentioned by @janb84, the tight coupling with config["environment"]["BUILDDIR"] and related settings means external usage still requires test framework. Not blocking for this PR, but worth considering for future refactoring if there's the requirement.
The config is written by the build system, so I don't think it is coupled with the TestFramework class itself.
Concept ACK
Code Review ACK fa75ef4
PR looks straight forward, moving binaries to util.py files, help make them reusable and not bound to the Test framework.
The config is written by the build system, so I don't think it is coupled with the TestFramework class itself.
Also confirmed that the config is written by the build system located at bitcoin/build/test/config.ini.
From my understanding, bitcoin/test/config.ini.in is a blueprint that gets filled in with real build info like [environment] and [components] after compilation.
Example after building:
[environment]
CLIENT_NAME=Bitcoin Core
CLIENT_BUGREPORT=https://github.com/bitcoin/bitcoin/issues
SRCDIR=/home/user-name/bitcoin
BUILDDIR=/home/user-name/bitcoin/build
EXEEXT=
RPCAUTH=/home/user-name/bitcoin/share/rpcauth/rpcauth.py
[components]
ENABLE_WALLET=true
ENABLE_CLI=true
313 | + default_filename = os.path.join( 314 | + config["environment"]["BUILDDIR"], 315 | + "bin", 316 | + binary + config["environment"]["EXEEXT"], 317 | + ) 318 | + setattr(paths, env_variable_name.lower(), os.getenv(env_variable_name, default=default_filename))
Just to clarify, i'm not saying that it is coupled to the test_framework, but that it is coupled to the config. For a followup I could see something like the code below, for this PR I like that it is more or less a code move.
Rough example; not ment to be a nit:
def get_binary_paths(bin_path, exe_ext):
"""Get paths of all binaries from environment variables or their default values"""
paths = types.SimpleNamespace()
binaries = {
"bitcoin": "BITCOIN_BIN",
"bitcoind": "BITCOIND",
"bitcoin-cli": "BITCOINCLI",
"bitcoin-util": "BITCOINUTIL",
"bitcoin-tx": "BITCOINTX",
"bitcoin-chainstate": "BITCOINCHAINSTATE",
"bitcoin-wallet": "BITCOINWALLET",
}
# Set paths to bitcoin core binaries allowing overrides with environment
# variables.
for binary, env_variable_name in binaries.items():
default_filename = os.path.join(
bin_path,
binary + exe_ext,
)
setattr(paths, env_variable_name.lower(), os.getenv(env_variable_name, default=default_filename))
thx, I'll leave as-is for now, but this can certainly be pushed in the future, if and once there is additional need to uncouple from the config
LGTM ACK fa75ef4328f638221bcf85fcbefa885122084622
321 | @@ -322,6 +322,13 @@ def get_binary_paths(config): 322 | return paths 323 | 324 | 325 | +def export_env_build_path(config):
nano nit:
consider adding a docstring to the new export_env_build_path function. Its purpose was clear in the old context, but a comment would be helpful now that it's a standalone utility.