qa: Use `sys.executable` when invoking other Python scripts

hebasto commented at 4:06 pm on December 19, 2024: member

This PR fixes the rpc_signer.py and wallet_signer.py functional tests on systems where python3 is not available in the PATH, causing the shebang #!/usr/bin/env python3 to fail.

Here are logs on NetBSD 10.0:

without this PR:

 0$ python3.12 ./build/test/functional/test_runner.py rpc_signer.py wallet_signer.py
 1Temporary test directory at /tmp/test_runner_₿_🏃_20241219_160538
 2Remaining jobs: [rpc_signer.py, wallet_signer.py --descriptors]
 31/2 - rpc_signer.py failed, Duration: 1 s
 4
 5stdout:
 62024-12-19T16:05:40.012000Z TestFramework (INFO): PRNG seed is: 1833166631173850775
 72024-12-19T16:05:40.012000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1
 82024-12-19T16:05:40.754000Z TestFramework (ERROR): Assertion failed
 9Traceback (most recent call last):
10  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 160, in try_rpc
11    fun(*args, **kwds)
12  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
13    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
14                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
16    raise JSONRPCException(response['error'], status)
17test_framework.authproxy.JSONRPCException: RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
18 (-1)
19
20During handling of the above exception, another exception occurred:
21
22Traceback (most recent call last):
23  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
24    self.run_test()
25  File "/home/hebasto/dev/bitcoin/build/test/functional/rpc_signer.py", line 72, in run_test
26    assert_raises_rpc_error(-1, 'fingerprint not found',
27  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 151, in assert_raises_rpc_error
28    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
29           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
30  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 166, in try_rpc
31    raise AssertionError(
32AssertionError: Expected substring not found in error message:
33substring: 'fingerprint not found'
34error message: 'RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
35'.
362024-12-19T16:05:40.756000Z TestFramework (INFO): Stopping nodes
372024-12-19T16:05:40.873000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1
382024-12-19T16:05:40.873000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1/test_framework.log
392024-12-19T16:05:40.873000Z TestFramework (ERROR): 
402024-12-19T16:05:40.873000Z TestFramework (ERROR): Hint: Call /home/hebasto/dev/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1' to consolidate all logs
412024-12-19T16:05:40.873000Z TestFramework (ERROR): 
422024-12-19T16:05:40.873000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
432024-12-19T16:05:40.873000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
442024-12-19T16:05:40.873000Z TestFramework (ERROR): 
45
46
47stderr:
48
49
50Remaining jobs: [wallet_signer.py --descriptors]
512/2 - wallet_signer.py --descriptors failed, Duration: 1 s
52
53stdout:
542024-12-19T16:05:40.014000Z TestFramework (INFO): PRNG seed is: 7530764367977090686
552024-12-19T16:05:40.014000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0
562024-12-19T16:05:40.526000Z TestFramework (ERROR): JSONRPC error
57Traceback (most recent call last):
58  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
59    self.run_test()
60  File "/home/hebasto/dev/bitcoin/build/test/functional/wallet_signer.py", line 66, in run_test
61    self.test_valid_signer()
62  File "/home/hebasto/dev/bitcoin/build/test/functional/wallet_signer.py", line 83, in test_valid_signer
63    self.nodes[1].createwallet(wallet_name='hww', disable_private_keys=True, descriptors=True, external_signer=True)
64  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_node.py", line 935, in createwallet
65    return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)
66           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
67  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
68    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
69                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
70  File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
71    raise JSONRPCException(response['error'], status)
72test_framework.authproxy.JSONRPCException: RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
73 (-1)
742024-12-19T16:05:40.528000Z TestFramework (INFO): Stopping nodes
752024-12-19T16:05:40.645000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0
762024-12-19T16:05:40.646000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0/test_framework.log
772024-12-19T16:05:40.646000Z TestFramework (ERROR): 
782024-12-19T16:05:40.646000Z TestFramework (ERROR): Hint: Call /home/hebasto/dev/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0' to consolidate all logs
792024-12-19T16:05:40.646000Z TestFramework (ERROR): 
802024-12-19T16:05:40.646000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
812024-12-19T16:05:40.646000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
822024-12-19T16:05:40.646000Z TestFramework (ERROR): 
83
84
85stderr:
86
87
88
89TEST                           | STATUS    | DURATION
90
91rpc_signer.py                  | ✖ Failed  | 1 s
92wallet_signer.py --descriptors | ✖ Failed  | 1 s
93
94ALL                            | ✖ Failed  | 2 s (accumulated) 
95Runtime: 1 s

with this PR:

 0$ python3.12 ./build/test/functional/test_runner.py rpc_signer.py wallet_signer.py   
 1Temporary test directory at /tmp/test_runner_₿_🏃_20241219_160011
 2Remaining jobs: [rpc_signer.py, wallet_signer.py --descriptors]
 31/2 - rpc_signer.py passed, Duration: 2 s
 4Remaining jobs: [wallet_signer.py --descriptors]
 52/2 - wallet_signer.py --descriptors passed, Duration: 3 s
 6
 7TEST                           | STATUS    | DURATION
 8
 9rpc_signer.py                  | ✓ Passed  | 2 s
10wallet_signer.py --descriptors | ✓ Passed  | 3 s
11
12ALL                            | ✓ Passed  | 5 s (accumulated) 
13Runtime: 3 s

hebasto added the label Tests on Dec 19, 2024

DrahtBot commented at 4:06 pm on December 19, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31541.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, stickies-v
Concept 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.

hebasto force-pushed on Dec 19, 2024

DrahtBot added the label CI failed on Dec 19, 2024

DrahtBot commented at 4:15 pm on December 19, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34664908840

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko commented at 5:08 pm on December 19, 2024: member

lgtm. even without netbsd, this is required to pass down the python version that was selected to run the tests. This is also used in all other places in the tests, like https://github.com/bitcoin/bitcoin/blob/bb57017b2945d5e0bbd95c7f1a9369a8ab7c6fcd/test/functional/test_runner.py#L632

qa: Use `sys.executable` when invoking other Python scripts

This change fixes tests on systems where `python3` is not available
in the `PATH`, causing the shebang `#!/usr/bin/env python3` to fail.

d38ade7bc4

hebasto force-pushed on Dec 19, 2024

hebasto marked this as ready for review on Dec 19, 2024

hebasto commented at 5:12 pm on December 19, 2024: member

lgtm. even without netbsd

Thanks! Rebased on the master branch.

DrahtBot removed the label CI failed on Dec 19, 2024

hebasto commented at 7:55 pm on December 19, 2024: member

Tracked in https://github.com/hebasto/bitcoin-core-nightly/issues/6.

fanquake commented at 11:05 am on December 20, 2024: member

Tracked in hebasto/bitcoin-core-nightly#6.

Unless there’s a good reason not too, it’d be good to track issues in our issue tracker, rather than some other repository. Otherwise they are not included in stats tracking, the metadata archives, Core Check dashboards, overall less discoverable etc

maflcko commented at 11:15 am on December 20, 2024: member

I think in this case it is fine, because each issue has a pull request with an extended description, so it will end up in the metadata backup. But anything should be fine by me.

maflcko commented at 11:16 am on December 20, 2024: member

lgtm ACK d38ade7bc409207bf425e05ee10b633b0aef4c36

In theory, the shebang could be removed from the mocks files, to better enforce this, but this could make one of the linters sad, so could be done in a follow-up.

hebasto commented at 11:51 am on December 20, 2024: member

Friendly ping Python connoisseur @stickies-v :)

tdb3 commented at 1:32 pm on January 2, 2025: contributor

Concept ACK

Ran both tests with test_runner on NetBSD 10.1 and Ubuntu 24.04. Both passed. Encountered a failure with tool_wallet on NetBSD that I want to check out before full ACK.

hebasto commented at 4:29 pm on January 2, 2025: member

Encountered a failure with tool_wallet on NetBSD that I want to check out before full ACK.

The issue with tool_wallet is fixed in #31543. It could be worked around by setting the LD_LIBRARY_PATH environment variable.

maflcko added this to the milestone 29.0 on Jan 2, 2025

maflcko removed this from the milestone 29.0 on Jan 2, 2025

BrandonOdiwuor commented at 9:46 am on January 7, 2025: contributor

Concept ACK

in test/functional/wallet_signer.py:25 in d38ade7bc4

23@@ -24,24 +24,15 @@ def add_options(self, parser):
24 
25     def mock_signer_path(self):

stickies-v commented at 10:42 am on January 16, 2025:

nit: these functions don’t return paths. mock_signer_command would be more appropriate. Alternatively, they could keep returning paths, and prepending the python binary could be a responsibility of the callsite (potentially through py_command() helper function.

in test/functional/rpc_signer.py:24 in d38ade7bc4

20@@ -20,10 +21,7 @@
21 class RPCSignerTest(BitcoinTestFramework):
22     def mock_signer_path(self):
23         path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py')
24-        if platform.system() == "Windows":
25-            return "py -3 " + path
26-        else:
27-            return path
28+        return sys.executable + " " + path

stickies-v commented at 10:58 am on January 16, 2025:

note: from https://docs.python.org/3/library/sys.html#sys.executable, there are no guarantees that sys.executable returns a Python path

A string giving the absolute path of the executable binary for the Python interpreter, on systems where this makes sense. If Python is unable to retrieve the real path to its executable, sys.executable will be an empty string or None.

It seems like this shouldn’t be an issue in normal circumstances, and I’m not sure if this should be considered user error if/when it happens, but it’s something to be aware of. Perhaps wrapping this line in a py_command(cmd) function and adding a warning log if sys.executable is empty might make future debugging easier?

maflcko commented at 11:35 am on January 16, 2025:

note: from https://docs.python.org/3/library/sys.html#sys.executable, there are no guarantees that sys.executable returns a Python path

For the purposes of functional test, it is guaranteed, because it is already used and enforced elsewhere. You can check this with git grep 'sys.exec' test/functional/.

stickies-v commented at 11:47 am on January 16, 2025:

Ah yes, that’s true, test_runner itself shouldn’t even work in that case.

stickies-v approved

stickies-v commented at 11:01 am on January 16, 2025: contributor

ACK d38ade7bc409207bf425e05ee10b633b0aef4c36 . I have a minor concern about sys.executable not being guaranteed to return a valid Python path, but this patch seems good enough as is so no blocker.

The mock scripts (e.g. mock/multi_signers.py) are executed from within functional tests such as wallet_signer.py. The mocks contain a shebang (#!/usr/bin/env python3), but shebangs are not well supported on Windows, which is why prior to this commit, py -3 was prefixed to the path.

Prior to this PR, on any platform, because the binary to be used to run these mocks (py on Windows, /usr/bin/env python3 on any other) is hardcoded, it can be different from the Python binary used to run the functional test. This can lead to to test failures or anomalies because of unexpected environment issues, or because the hardcoded binaries are not in fact available on the user’s system.

Fix this by running the mock scripts with the same binary as the test runner.

DrahtBot requested review from tdb3 on Jan 16, 2025

DrahtBot requested review from BrandonOdiwuor on Jan 16, 2025

qa: Use sys.executable when invoking other Python scripts #31541

Code Coverage & Benchmarks

Reviews

qa: Use `sys.executable` when invoking other Python scripts #31541