💬 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
...
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869409094)
I'd say this is unrelated to flushing. Ignoring the return code in all tasks can mean that even the tasks where the detection is working are going to silently break tomorrow. As mentioned previously in the thread you closed, I think this is brittle.
To clarify, my feedback not just applies to the line I commented on, but it applies to all code where the return code is silently ignored and where it could lead to silent breakage.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869409094)
I'd say this is unrelated to flushing. Ignoring the return code in all tasks can mean that even the tasks where the detection is working are going to silently break tomorrow. As mentioned previously in the thread you closed, I think this is brittle.
To clarify, my feedback not just applies to the line I commented on, but it applies to all code where the return code is silently ignored and where it could lead to silent breakage.