💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893670004)
So, the user must have a knowledge whether his Bitcoin Core was compiled with or without multiprocess and start either `bitcoin -M gui` or `bitcoin -m gui`. Could we spare them this? The `bitcoin` executable has that knowledge.
Then the user can run just `bitcoin gui` and under the hood either `bitcoin-qt` or `bitcoin-gui` will be executed, depending on how it was compiled? Or maybe we can probe at run time whether the executables `bitcoin-qt` and `bitcoin-gui` exist and run whichever is pres
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893670004)
So, the user must have a knowledge whether his Bitcoin Core was compiled with or without multiprocess and start either `bitcoin -M gui` or `bitcoin -m gui`. Could we spare them this? The `bitcoin` executable has that knowledge.
Then the user can run just `bitcoin gui` and under the hood either `bitcoin-qt` or `bitcoin-gui` will be executed, depending on how it was compiled? Or maybe we can probe at run time whether the executables `bitcoin-qt` and `bitcoin-gui` exist and run whichever is pres
...
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893770067)
I think this is the same check as above: `argv0.find('/') == std::string_view::npos`, right? Would be good to use the same expression in both locations.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893770067)
I think this is the same check as above: `argv0.find('/') == std::string_view::npos`, right? Would be good to use the same expression in both locations.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893702291)
If you switch this to doxygen compatible comment it will be attached to the doxygen generated docs about `ExecCommand()`. Would be nice to format the arguments' descriptions with `@param[in] ...`.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893702291)
If you switch this to doxygen compatible comment it will be attached to the doxygen generated docs about `ExecCommand()`. Would be nice to format the arguments' descriptions with `@param[in] ...`.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893696352)
This adds a trailing newline, but the above 2 `tfm::format()` calls don't. I think they should.
I think the `tfm::format(..., FormatParagraph(LicenseInfo()))` call adds a trailing `\n` because `LicenseInfo()` has it, but I did not check.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893696352)
This adds a trailing newline, but the above 2 `tfm::format()` calls don't. I think they should.
I think the `tfm::format(..., FormatParagraph(LicenseInfo()))` call adds a trailing `\n` because `LicenseInfo()` has it, but I did not check.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893709992)
I find the names confusing because I would assume this will execute `argv0 args[0] args[1] args[2] ...`. Maybe rename `argv0` to `parent` or `wrapper_prog`.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893709992)
I find the names confusing because I would assume this will execute `argv0 args[0] args[1] args[2] ...`. Maybe rename `argv0` to `parent` or `wrapper_prog`.
💬 maflcko commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1893810274)
If this is fine, you could just drop the line (and leave it to the host to clean up). If not, I am not sure how to fix it.
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1893810274)
If this is fine, you could just drop the line (and leave it to the host to clean up). If not, I am not sure how to fix it.
👍 TheCharlatan approved a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517335576)
Nice improvements, Re-ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517335576)
Nice improvements, Re-ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b
🤔 hebasto reviewed a pull request: "ci: Allow build dir on CI host"
(https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2517330988)
Approach ACK faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21, tested with https://github.com/bitcoin/bitcoin/pull/31176 built on top of this PR.
(https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2517330988)
Approach ACK faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21, tested with https://github.com/bitcoin/bitcoin/pull/31176 built on top of this PR.
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893810387)
An extra brace:
```suggestion
echo "BASE_BUILD_DIR=${RUNNER_TEMP}/build-asan" >> "$GITHUB_ENV"
```
?
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893810387)
An extra brace:
```suggestion
echo "BASE_BUILD_DIR=${RUNNER_TEMP}/build-asan" >> "$GITHUB_ENV"
```
?
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893812448)
Should the definition of `BASE_BUILD_DIR` from https://github.com/bitcoin/bitcoin/blob/faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21/ci/test/03_test_script.sh#L118-L119
be moved before this first usage?
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893812448)
Should the definition of `BASE_BUILD_DIR` from https://github.com/bitcoin/bitcoin/blob/faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21/ci/test/03_test_script.sh#L118-L119
be moved before this first usage?
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893810563)
nit: I slightly prefer `${{ runner.temp }}` over `${RUNNER_TEMP}` because it expands in the log, which feels convenient.
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893810563)
nit: I slightly prefer `${{ runner.temp }}` over `${RUNNER_TEMP}` because it expands in the log, which feels convenient.
💬 maflcko commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556797216)
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.
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556797216)
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.
🤔 l0rinc requested changes to a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517230492)
Thanks for the cleanup, please consider a few simplification and unification recommendations
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517230492)
Thanks for the cleanup, please consider a few simplification and unification recommendations
💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893758544)
it's a small change, please consider reformatting the whole file
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893758544)
it's a small change, please consider reformatting the whole file
💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893781576)
we're getting `Functional-style cast is used instead of a C++ cast` warning now, what if we used character literals instead:
```suggestion
string(REGEX REPLACE "[^\n][^\n]" "'\\\\x\\0'," formatted_bytes "${formatted_bytes}")
```
which would look like:
```C++
'\x5b','\x5c','\x6e','\x61','\x6b','\x65','\x64','\x5d',
```
instead of
```C++
char(0x5b),char(0x5c),char(0x6e),char(0x61),char(0x6b),char(0x65),char(0x64),char(0x5d),
```
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893781576)
we're getting `Functional-style cast is used instead of a C++ cast` warning now, what if we used character literals instead:
```suggestion
string(REGEX REPLACE "[^\n][^\n]" "'\\\\x\\0'," formatted_bytes "${formatted_bytes}")
```
which would look like:
```C++
'\x5b','\x5c','\x6e','\x61','\x6b','\x65','\x64','\x5d',
```
instead of
```C++
char(0x5b),char(0x5c),char(0x6e),char(0x61),char(0x6b),char(0x65),char(0x64),char(0x5d),
```
💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893750818)
does this sorting (here and in unitester.cpp) serve some purpose of could we use natural ordering?
We can't see this way that e.g. `fail43` is missing and adding new lines would get confusing this way (maybe even adding a 0 prefix for <10):
<details>
<summary>Details</summary>
```C++
if(BUILD_TESTS)
include(GenerateHeaders)
generate_header_from_json(test/fail1.json)
generate_header_from_json(test/fail2.json)
generate_header_from_json(test/fail3.json)
generate_header_from_
...
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893750818)
does this sorting (here and in unitester.cpp) serve some purpose of could we use natural ordering?
We can't see this way that e.g. `fail43` is missing and adding new lines would get confusing this way (maybe even adding a 0 prefix for <10):
<details>
<summary>Details</summary>
```C++
if(BUILD_TESTS)
include(GenerateHeaders)
generate_header_from_json(test/fail1.json)
generate_header_from_json(test/fail2.json)
generate_header_from_json(test/fail3.json)
generate_header_from_
...
💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893811557)
I understand why a macro is the smallest change to make `runtest` happy, but I think we can avoid that weird string parsing and introduce enums instead:
```C++
enum class TestType { Fail, Pass, RoundTrip };
```
and define the test as:
```C++
inline constexpr std::map<std::string_view, TestType> tests{
{json_tests::fail1, TestType::Fail},
{json_tests::fail2, TestType::Fail},
{json_tests::fail3, TestType::Fail},
...
```
which could simplify `runtest` to:
```C++
static voi
...
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893811557)
I understand why a macro is the smallest change to make `runtest` happy, but I think we can avoid that weird string parsing and introduce enums instead:
```C++
enum class TestType { Fail, Pass, RoundTrip };
```
and define the test as:
```C++
inline constexpr std::map<std::string_view, TestType> tests{
{json_tests::fail1, TestType::Fail},
{json_tests::fail2, TestType::Fail},
{json_tests::fail3, TestType::Fail},
...
```
which could simplify `runtest` to:
```C++
static voi
...
💬 maflcko commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556799506)
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.
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556799506)
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.
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820716)
I want to keep the changes minimal. Happy to review a follow-up doing this, but I'll leave this as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820716)
I want to keep the changes minimal. Happy to review a follow-up doing this, but I'll leave this as-is for now.
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820757)
> we're getting `Functional-style cast is used instead of a C++ cast` warning now, what if we used character literals instead:
Where? Functional-style casts are recommended by the dev notes, so if you want to change that, it would be good to change the dev notes first.
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820757)
> we're getting `Functional-style cast is used instead of a C++ cast` warning now, what if we used character literals instead:
Where? Functional-style casts are recommended by the dev notes, so if you want to change that, it would be good to change the dev notes first.