Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
4c06658 to
5852983
Compare
|
Errors all appear to be |
|
I noticed |
|
Maybe rebase after #197 for easier review? |
5852983 to
e0c1fb0
Compare
Sure! Rebased. |
|
Assuming the netbsd job still fails, I guess there are different directions we may go with this issue:
And regardless of these, we may want to add a try_compile test and error with suggested workarounds along the lines of #205 |
FWIW, build with depends succeeds: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/17482518540/job/49655572035. |
|
Could rebase for fresh CI logs? |
e0c1fb0 to
7056dce
Compare
Sure thing! Done. |
7056dce to
51a1940
Compare
|
For reference, the full error is: |
|
I built the updated |
1d93480 to
a5a4d6d
Compare
a5a4d6d to
1e37f77
Compare
|
CI is green now. Undrafted. |
Officially, GCC has supported the As a result, |
1e37f77 to
afef05f
Compare
|
The implementation of the A side note: all |
afef05f to
d6f80ca
Compare
|
Reworked to test all supported NetBSD releases. |
ryanofsky
left a comment
There was a problem hiding this comment.
Code review d6f80ca. Thanks for the update and nice to see this working! If you can add a few comments to explain (whatever is known about) a few mysterious things in this config, I think it would be ok to merge this. Specific questions I have are below.
| cvs -danoncvs@anoncvs.NetBSD.org:/cvsroot checkout -P \ | ||
| pkgsrc/devel/capnproto \ | ||
| pkgsrc/devel/libtool-base \ | ||
| pkgsrc/devel/pkgconf \ | ||
| pkgsrc/devel/zlib \ | ||
| pkgsrc/lang/gcc15 \ | ||
| pkgsrc/mk \ | ||
| pkgsrc/pkgtools \ | ||
| pkgsrc/security/openssl \ | ||
| pkgsrc/sysutils/install-sh/files |
There was a problem hiding this comment.
In commit "ci: Add NetBSD job" (d6f80ca)
Can you add a comment about where this list of dependencies comes from? I would like have some idea of how it can be updated in the future in the case of a new release or breakage.
I see openssl and zlib included at the bottom of https://cvsweb.netbsd.org/bsdweb.cgi/pkgsrc/devel/capnproto/Makefile?rev=1.11 so it makes sense they need to be included in the CVS checkout. But i'm not sure about the other ones. Especially gcc15 which seems to be checked out but not ever mentioned in the logs after the cvs checkout. So maybe it is not being used? The gcc15 version is also surprising since gcc14 seems to be used in other places this PR.
If you don't have answers to all the questions here that is fine, it would just help to have a comment saying where this comes from, so it is clear which parts of the netbsd config were intentionally chosen and which parts we are unsure about.
There was a problem hiding this comment.
Can you add a comment about where this list of dependencies comes from?
The entire run option is commented:
libmultiprocess/.github/workflows/ci.yml
Lines 33 to 35 in d6f80ca
Especially gcc15 which seems to be checked out but not ever mentioned in the logs after the cvs checkout. So maybe it is not being used? The gcc15 version is also surprising since gcc14 seems to be used in other places this PR.
I was surprised as well. However, the build fails without lang/gcc15:
make: "/usr/pkgsrc/mk/compiler/gcc.mk" line 311: Could not find ../../lang/gcc15/version.mk
make: Fatal errors encountered -- cannot continue
make: stopped in /usr/pkgsrc/devel/capnproto
The same applies to all other packages.
There was a problem hiding this comment.
re: #196 (comment)
I guess my question is literally where did this list of dependencies come from originally? Did you look at netbsd package information at a web url? Look at cvs sources? Have an LLM generate it? Just arrive at it by trial and error? It would be helpful to know more so we can know how to maintain this.
And thanks for explaining gcc15 dependency and providing the error. It would be good to have a comment explaining discrepancy with gcc 14 like: "gcc15 is referenced here because the pkgsrc framework requires lang/gcc15/version.mk to exist during the "make install" step below, even though we compile our project with gcc14."
| CI_DESC="CI config for NetBSD" | ||
| CI_DIR=build-netbsd | ||
| export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter" | ||
| export CXX="/usr/pkg/gcc14/bin/g++" |
There was a problem hiding this comment.
In commit "ci: Add NetBSD job" (d6f80ca)
Is it necessary to hardcode gcc 14 here? If the gcc version in this file needs to be kept in sync with the gcc version ci.yml it would be helpful to have a comment both places stating this. Or if there is a way to use a default system compiler or a default gcc version and not hardcode any version that would seem even nicer.
There was a problem hiding this comment.
Is it necessary to hardcode gcc 14 here? ... if there is a way to use a default system compiler or a default gcc version and not hardcode any version that would seem even nicer.
On NetBSD 9.4, the default compiler is GCC 7.5.0, and the build fails at the configuration stage.
On NetBSD 10.1, the default compiler is GCC 10.5.0, and the build fails as well:
[6/7] Building CXX object CMakeFiles/multiprocess.dir/src/mp/proxy.cpp.o
FAILED: [code=1] CMakeFiles/multiprocess.dir/src/mp/proxy.cpp.o
/usr/bin/c++ -I/home/runner/work/libmultiprocess/libmultiprocess/build-netbsd/include -I/home/runner/work/libmultiprocess/libmultiprocess/include -isystem /usr/pkg/include -Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter -std=gnu++2a -pthread -MD -MT CMakeFiles/multiprocess.dir/src/mp/proxy.cpp.o -MF CMakeFiles/multiprocess.dir/src/mp/proxy.cpp.o.d -o CMakeFiles/multiprocess.dir/src/mp/proxy.cpp.o -c /home/runner/work/libmultiprocess/libmultiprocess/src/mp/proxy.cpp
In file included from /home/runner/work/libmultiprocess/libmultiprocess/src/mp/proxy.cpp:8:
/home/runner/work/libmultiprocess/libmultiprocess/include/mp/proxy-types.h:527:2: error: extra ‘;’ [-Werror=pedantic]
527 | };
| ^
cc1plus: all warnings being treated as errors
ninja: build stopped: cannot make progress due to previous errors.
There was a problem hiding this comment.
If the gcc version in this file needs to be kept in sync with the gcc version
ci.ymlit would be helpful to have a comment both places stating this.
Thanks! Updated.
There was a problem hiding this comment.
re: #196 (comment)
Thanks for providing the errors. It appears the reason for hardcoding gcc14 is that if you don't hardcode it, netbsd uses much older versions of gcc that don't recognize c++20 or fail with -Werror=pedantic due to a trailing semicolon.
I guess my next question would be why use gcc14 instead of another version of GCC? Was the choice based on trial and error, or any particular reasoning?
I think my suggestion would be to add a comment to netbsd.bash explaining the need for hardcoding and the choice of hardcoded version. For example, "Hardcode GCC 14, since default GCC versions installed by netbsd are older and may not be compatible with libmultiprocess. GCC 14 was chosen because it's the latest compiler available on all versions of netbsd that we test. Note that the GCC version specified here must match the version specified in pkg_add in ci.yml." I don't know if this comment is accurate, but whatever version of this comment that would be accurate would be helpful so we know how this needs to be maintained going forward.
d6f80ca to
47a5d47
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 47a5d47. Thanks for providing all the additional error information. That was really helpful and cleared up most of my confusion. I still think it would be good to add more comments here explaining the reasons behind the configuration so the files can be updated less confusion and guesswork. I suggested two comments below, but would be happy to merge the PR as-is if that would be better or more convenient.
| cvs -danoncvs@anoncvs.NetBSD.org:/cvsroot checkout -P \ | ||
| pkgsrc/devel/capnproto \ | ||
| pkgsrc/devel/libtool-base \ | ||
| pkgsrc/devel/pkgconf \ | ||
| pkgsrc/devel/zlib \ | ||
| pkgsrc/lang/gcc15 \ | ||
| pkgsrc/mk \ | ||
| pkgsrc/pkgtools \ | ||
| pkgsrc/security/openssl \ | ||
| pkgsrc/sysutils/install-sh/files |
There was a problem hiding this comment.
re: #196 (comment)
I guess my question is literally where did this list of dependencies come from originally? Did you look at netbsd package information at a web url? Look at cvs sources? Have an LLM generate it? Just arrive at it by trial and error? It would be helpful to know more so we can know how to maintain this.
And thanks for explaining gcc15 dependency and providing the error. It would be good to have a comment explaining discrepancy with gcc 14 like: "gcc15 is referenced here because the pkgsrc framework requires lang/gcc15/version.mk to exist during the "make install" step below, even though we compile our project with gcc14."
| CI_DESC="CI config for NetBSD" | ||
| CI_DIR=build-netbsd | ||
| export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter" | ||
| export CXX="/usr/pkg/gcc14/bin/g++" |
There was a problem hiding this comment.
re: #196 (comment)
Thanks for providing the errors. It appears the reason for hardcoding gcc14 is that if you don't hardcode it, netbsd uses much older versions of gcc that don't recognize c++20 or fail with -Werror=pedantic due to a trailing semicolon.
I guess my next question would be why use gcc14 instead of another version of GCC? Was the choice based on trial and error, or any particular reasoning?
I think my suggestion would be to add a comment to netbsd.bash explaining the need for hardcoding and the choice of hardcoded version. For example, "Hardcode GCC 14, since default GCC versions installed by netbsd are older and may not be compatible with libmultiprocess. GCC 14 was chosen because it's the latest compiler available on all versions of netbsd that we test. Note that the GCC version specified here must match the version specified in pkg_add in ci.yml." I don't know if this comment is accurate, but whatever version of this comment that would be accurate would be helpful so we know how this needs to be maintained going forward.
47a5d47 to
861da39
Compare
|
Thanks for the suggested comments, which were gratefully incorporated. |
| pkgsrc/devel/libtool-base \ | ||
| pkgsrc/devel/pkgconf \ | ||
| pkgsrc/devel/zlib \ | ||
| `# gcc15 is referenced here because the pkgsrc framework requires lang/gcc15/version.mk to exist` \ |
There was a problem hiding this comment.
In commit "ci: Add NetBSD job" (861da39)
Interesting, have not seen `# comment` used before in bash, but makes sense here
…8f7d 1fc65008f7d Merge bitcoin-core/libmultiprocess#237: Made SpawnProcess() behavior safe post fork() 5205a87cd90 test: check SpawnProcess post-fork safety 69652f0edfa Precompute argv before fork in SpawnProcess 30a8681de62 SpawnProcess: avoid fd leak on close failure d0fc1081d09 Merge bitcoin-core/libmultiprocess#196: ci: Add NetBSD job 7b171f45bfc Merge bitcoin-core/libmultiprocess#234: doc: Fix typos and grammar in documentation and comments 861da39cae9 ci: Add NetBSD job 458745e3940 Fix various typos, spelling mistakes, and grammatical errors in design.md and source code comments. 585decc8561 Merge bitcoin-core/libmultiprocess#236: ci: Install binary package `capnproto` on OpenBSD instead of building it 14e926a3ff3 refactor: extract MakeArgv helper 1ee909393f4 ci: Install binary package `capnproto` on OpenBSD instead of building it 470fc518d4b Merge bitcoin-core/libmultiprocess#230: cmake: add ONLY_CAPNP target_capnp_sources option 2d8886f26c4 Merge bitcoin-core/libmultiprocess#228: Add versions.md and version.h files describing version branches and tags c1838be565d Merge bitcoin-core/libmultiprocess#225: Improve and document act support a173f1704ce Merge bitcoin-core/libmultiprocess#223: ci: Replace nix-shell with equivalent nix develop command 625eaca42fb Merge bitcoin-core/libmultiprocess#229: Design Documentation Update cc234be73a6 Design doc update 81c652687b8 cmake: add ONLY_CAPNP target_capnp_sources option 6e01d2d766e Add versions.md and version.h files describing version branches and tags 4e3f8fa0d2c doc: add instructions for using act 81712ff6bbf ci: disable KVM and sandbox inside act containers 18a2237a8ef ci: Replace nix-shell with equivalent nix develop command git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1fc65008f7d64161e84c08cbd93109a23dd6a1e9
…misc.py timeout 7562e2a Squashed 'src/ipc/libmultiprocess/' changes from a4f92969649..1fc65008f7d (Ryan Ofsky) Pull request description: Includes: - bitcoin-core/libmultiprocess#229 - bitcoin-core/libmultiprocess#223 - bitcoin-core/libmultiprocess#225 - bitcoin-core/libmultiprocess#228 - bitcoin-core/libmultiprocess#230 - bitcoin-core/libmultiprocess#236 - bitcoin-core/libmultiprocess#234 - bitcoin-core/libmultiprocess#196 - bitcoin-core/libmultiprocess#237 The last change bitcoin-core/libmultiprocess#237 is expected to fix issue #34187 occasional `rpc_misc.py` test hang. The other changes are just documentation & ci updates. The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh) ACKs for top commit: sedited: ACK 73d0fe6 Tree-SHA512: 82fb2973b8fb5e792dcff1adde0b158ac67cc6a9aeef4465e0bbdfbef88e41d44d9bad53b1d467be47bf594befa02bd8e7829c0a0bc67aaaeb4ca1e09b672a32
The master branch @ 1b8d4a6 fails to build on NetBSD.
See: https://github.com/hebasto/libmultiprocess/actions/runs/17175941246/job/48731811650