💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516945982)
> CMake picks the Python from .venv
I'm not using a `.venv`. It's Python installed with `pyenv`.
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516945982)
> CMake picks the Python from .venv
I'm not using a `.venv`. It's Python installed with `pyenv`.
💬 willcl-ark commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516958005)
Thanks @hebasto for the double-check. It has helped me work out what was going wrong for me (I think)...
Looks like I had not bumped my python version in the venv since #30527 so I was still running 3.9 in my venv. This caused the `cmake` check to (silently) _skip_ that version during configure and fallback to a "system" version (3.13).
This was then combined with me manually exporting `PYTHONPATH` in this dir (to make `Pyright` perform better in `nvim`), which was causing a right mismatch
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516958005)
Thanks @hebasto for the double-check. It has helped me work out what was going wrong for me (I think)...
Looks like I had not bumped my python version in the venv since #30527 so I was still running 3.9 in my venv. This caused the `cmake` check to (silently) _skip_ that version during configure and fallback to a "system" version (3.13).
This was then combined with me manually exporting `PYTHONPATH` in this dir (to make `Pyright` perform better in `nvim`), which was causing a right mismatch
...
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869221388)
Generally, I am not sure if bash is the right language to implement something like this. Silently ignoring possible errors doesn't seem like a great approach. Other languages, such as Rust or Python are a bit more explicit in documenting when a return code is ignored.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869221388)
Generally, I am not sure if bash is the right language to implement something like this. Silently ignoring possible errors doesn't seem like a great approach. Other languages, such as Rust or Python are a bit more explicit in documenting when a return code is ignored.
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869230414)
Then it would be unclear whether the traffic was generated by unit tests, functional tests or fuzz tests. So I start/stop a separate monitoring for each of those set of tests. I perceived this information useful when investigating. It is already tough that it is not known which test generated the traffic.
Somehow I do not like reading from the "live" file while it is being written. `-U` would help not to miss data, but it might happen that by the time we read it the last packet is still being
...
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869230414)
Then it would be unclear whether the traffic was generated by unit tests, functional tests or fuzz tests. So I start/stop a separate monitoring for each of those set of tests. I perceived this information useful when investigating. It is already tough that it is not known which test generated the traffic.
Somehow I do not like reading from the "live" file while it is being written. `-U` would help not to miss data, but it might happen that by the time we read it the last packet is still being
...
👍 danielabrozzoni approved a pull request: "refactor: Move GuessVerificationProgress into ChainstateManager"
(https://github.com/bitcoin/bitcoin/pull/31393#pullrequestreview-2478006074)
re-ACK 5555ed0c3b8d1beecb2785dc8cbe10c1693ca290
(https://github.com/bitcoin/bitcoin/pull/31393#pullrequestreview-2478006074)
re-ACK 5555ed0c3b8d1beecb2785dc8cbe10c1693ca290
💬 danielabrozzoni commented on pull request "refactor: Move GuessVerificationProgress into ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1869153515)
nit: you also deleted a whitespace here, and now the line is indented with only 3 whitespaces, is this on purpose?
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1869153515)
nit: you also deleted a whitespace here, and now the line is indented with only 3 whitespaces, is this on purpose?
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869235192)
> > Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
>
> That is interesting, because all tasks should be running on workers that are set up identically.
I noticed those tasks print:
> Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869235192)
> > Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
>
> That is interesting, because all tasks should be running on workers that are set up identically.
I noticed those tasks print:
> Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg
💬 maflcko commented on pull request "refactor: Move GuessVerificationProgress into ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1869236012)
It is done by clang-format
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1869236012)
It is done by clang-format
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869246798)
> > That is interesting, because all tasks should be running on workers that are set up identically.
>
> I noticed those tasks print:
>
> > Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg
That should be identical for all tasks as well. See:
* https://cirrus-ci.com/task/5454797781860352?logs=ci#L186
* https://cirrus-ci.com/task/5314060293505024?logs=ci#L184
The only difference should be that one is bare metal and the other is KVM, but I don't see how
...
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869246798)
> > That is interesting, because all tasks should be running on workers that are set up identically.
>
> I noticed those tasks print:
>
> > Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg
That should be identical for all tasks as well. See:
* https://cirrus-ci.com/task/5454797781860352?logs=ci#L186
* https://cirrus-ci.com/task/5314060293505024?logs=ci#L184
The only difference should be that one is bare metal and the other is KVM, but I don't see how
...
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869261700)
Hmm, right. Then I have no idea why `kill` does not work on some. It is ok as long as it works on at least one.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869261700)
Hmm, right. Then I have no idea why `kill` does not work on some. It is ok as long as it works on at least one.
✅ fanquake closed an issue: "fuzz, parse_iso8601: attempt to dereference an end-of-stream istreambuf_iterator"
(https://github.com/bitcoin/bitcoin/issues/28917)
(https://github.com/bitcoin/bitcoin/issues/28917)
🚀 fanquake merged a pull request: "util: Drop boost posix_time in ParseISO8601DateTime"
(https://github.com/bitcoin/bitcoin/pull/31391)
(https://github.com/bitcoin/bitcoin/pull/31391)
💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517101949)
> > CMake picks the Python from .venv
>
> I'm not using a `.venv`. It's Python installed with `pyenv`.
My apologies.
Adding `-DPython3_FIND_FRAMEWORK=LAST` should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517101949)
> > CMake picks the Python from .venv
>
> I'm not using a `.venv`. It's Python installed with `pyenv`.
My apologies.
Adding `-DPython3_FIND_FRAMEWORK=LAST` should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517114493)
> Adding -DPython3_FIND_FRAMEWORK=LAST should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version?
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517114493)
> Adding -DPython3_FIND_FRAMEWORK=LAST should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version?
💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517149116)
> > Adding -DPython3_FIND_FRAMEWORK=LAST should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
>
> Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version?
The behaviour is extensively documented:
1. From the [`FindPython3`](https://cmake.org/cmake/help/latest/module/FindPython3.html) module docs:
> On
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517149116)
> > Adding -DPython3_FIND_FRAMEWORK=LAST should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
>
> Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version?
The behaviour is extensively documented:
1. From the [`FindPython3`](https://cmake.org/cmake/help/latest/module/FindPython3.html) module docs:
> On
...
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869367232)
Ignoring the return status of `tcpdump` here is deliberate because some VMs, the ones that couldn't stop `tcpdump` failed to read the unflushed file with an error like "tcpdump: truncated dump file; tried to read 4 file header bytes, only got 0". Now they don't due to the added `-U`, hmm...
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869367232)
Ignoring the return status of `tcpdump` here is deliberate because some VMs, the ones that couldn't stop `tcpdump` failed to read the unflushed file with an error like "tcpdump: truncated dump file; tried to read 4 file header bytes, only got 0". Now they don't due to the added `-U`, hmm...
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517179160)
> The behaviour is extensively documented:
Sure, but none of that means anything to a (new) developer, who doesn't care about the intricacies of the build system. It's just unintuituve that CMake would pick some version of Python, other than the one their shell/sytem is (globally) configured to use.
We even suggest using `pyenv` in our [documentation](https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines), so it makes even less sense that this workflow w
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517179160)
> The behaviour is extensively documented:
Sure, but none of that means anything to a (new) developer, who doesn't care about the intricacies of the build system. It's just unintuituve that CMake would pick some version of Python, other than the one their shell/sytem is (globally) configured to use.
We even suggest using `pyenv` in our [documentation](https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines), so it makes even less sense that this workflow w
...
🤔 l0rinc reviewed a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2478197206)
I find a lot of negations very confusing, but I guess it's existing functionality, so we might as well embrace it
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2478197206)
I find a lot of negations very confusing, but I guess it's existing functionality, so we might as well embrace it
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869266813)
I'm not sure I understand how `-noversion` is supposed to work.
I tried changing `test/functional/feature_help.py` to:
```python
def run_test(self):
self.log.info("Start bitcoin with -h for help text")
self.nodes[0].start(extra_args=['-h', '-noversion']) # added -noversion
# Node should exit immediately and output help to stdout.
output, _ = self.get_node_output(ret_code_expected=0)
assert b'Options' in output
assert b'version' not i
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869266813)
I'm not sure I understand how `-noversion` is supposed to work.
I tried changing `test/functional/feature_help.py` to:
```python
def run_test(self):
self.log.info("Start bitcoin with -h for help text")
self.nodes[0].start(extra_args=['-h', '-noversion']) # added -noversion
# Node should exit immediately and output help to stdout.
output, _ = self.get_node_output(ret_code_expected=0)
assert b'Options' in output
assert b'version' not i
...
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869307947)
nit: `len(debug_logs) > 0 && len(debug_logs) < 2` is basically `len(debug_logs) == 1`.
Which could likely be consolidated to a switch, something like:
```python
debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
match len(debug_logs):
case 0: chain = 'regtest'
case 1: chain = re.search(r'node0/(.+?)/debug\.log$', debug_logs[0].as_posix()).group(1)
case _: raise AssertionError(f"Max one debug.log is supported, found several:\n\t" + "\n\t".jo
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869307947)
nit: `len(debug_logs) > 0 && len(debug_logs) < 2` is basically `len(debug_logs) == 1`.
Which could likely be consolidated to a switch, something like:
```python
debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
match len(debug_logs):
case 0: chain = 'regtest'
case 1: chain = re.search(r'node0/(.+?)/debug\.log$', debug_logs[0].as_posix()).group(1)
case _: raise AssertionError(f"Max one debug.log is supported, found several:\n\t" + "\n\t".jo
...