fanquake
commented at 2:22 PM on September 1, 2022:
member
Now that CXXFLAGS are back in user control, I don't think there's a
reason to no-longer use our warning flags when CXXFLAGS has been
overriden (this includes, by default, when building from depends).
Anyone can suppress warnings from third-party code by
passing the relevant -Wno- options in CXXFLAGS.
theuni
commented at 7:03 PM on September 13, 2022:
member
As @fanquake mentioned, some of these are only warnings in older compilers. I'm going through them to double-check that they are indeed non-issues, as opposed to missed warnings in newer compilers.
(I'm ignoring the sdt.h warnings for now and focusing on what's in our code. I will look at that header next.)
script/descriptor.cpp:1555:21: error: loop variable 'keyspan' of type 'const Span<const unsigned char>' creates a copy from type 'const Span<const unsigned char>' [-Werror,-Wrange-loop-analysis]
for (const auto keyspan : match->second) {
tl;dr: copies are allowed for POD types less than 64bytes.
We can confirm this by adding a dummy char foo[65] to Span, which causes newer clang to warn as expected.
That seems reasonable enough, but I think we may as well just fix it up to take a const ref to remove any ambiguity.
for this one:
script/descriptor.cpp: In function ‘std::unique_ptr<{anonymous}::DescriptorImpl> {anonymous}::InferScript(const CScript&, {anonymous}::ParseScriptContext, const SigningProvider&)’:
script/descriptor.cpp:1262:17: error: ‘<anonymous>’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
return {};
Though I'm having trouble recreating a minimal test-case that fails with gcc9 and succeeds with gcc10.
Changing these from return {} to return std::nullopt avoids the problem and makes the code more clear too, so again, I suggest simply appeasing the dumb compiler because there's not really a reason not to.
For this one:
fs.cpp: In member function ‘bool fsbridge::FileLock::TryLock()’:
fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Werror=missing-field-initializers]
129 | _OVERLAPPED overlapped = {0};
This one looks legitimate. Suggest fixing by initializing to {}.
For this one:
test/fuzz/float.cpp: In function ‘void float_fuzz_target(FuzzBufferType)’:
test/fuzz/float.cpp:50:40: error: ‘tmp’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
uint64_t encoded = EncodeDouble(d);
(This applies to x86_64 as well)
Again, gcc10 is smart enough to figure this out, but gcc9 complains. Initializing to zero is harmless, so again I suggest just initializing to 0 to quiet the old compilers.
theuni
commented at 8:38 PM on September 13, 2022:
member
I also pushed a patch for systemtap in depends that gets rid of those compiler warnings. From what I can tell, the variadic is completely unused there, but I'm not at all confident in that change yet.
theuni
commented at 8:38 PM on September 13, 2022:
member
Concept ACK, btw :)
fanquake force-pushed on Sep 14, 2022
fanquake
commented at 11:00 AM on September 14, 2022:
member
This one looks legitimate. Suggest fixing by initializing to {}.
I had opened #26006, to see if anyone would jump in with the change, but at this point will just PR your commit separate to this.
For the remaining of the changes, I've rebased on top of your branch I think the changes are fairly uncontreversial (aside from sdt.h), and are better than adding -Wno-x for older compilers / CI jobs, which is better again than going the #pragma in our source code route. Although given they are changes to code, they can also be split out of here if wanted.
I also pushed a patch for systemtap in depends that gets rid of those compiler warnings. From what I can tell, the variadic is completely unused there, but I'm not at all confident in that change yet.
0xB10C
commented at 12:36 PM on September 14, 2022:
contributor
I also pushed a patch for systemtap in depends that gets rid of those compiler warnings. From what I can tell, the variadic is completely unused there, but I'm not at all confident in that change yet.
I too think that we don't use the variadic and it's Ok to drop it in the patch:
We use, for example, the DTRACE_PROBE6(context, event, a, b, c, d, e, f) macro which expands to STAP_PROBE6(provider,probe,parm1,parm2,parm3,parm4,parm5,parm6) expanding too _SDT_PROBE(provider, name, 6, (arg1, arg2, arg3, arg4, arg5, arg6)) and again expanding too (_SDT_ASM_BODY(provider, name, _SDT_ASM_ARGS, (n)) which is defined as _SDT_ASM_BODY(provider, name, pack_args, args, ...) containing the variadic. We only fill in the provider, name, pack_args, args and don't use the variadic.
The the tracepoint tests in this task succeed. Also, for reference, this also came up in #25528 (comment) and the variadic was only recently re-added in sytemtap@ecab2afe. If needed, we could also have our own sdt.h at some point with everything we don't use thrown out.
fanquake
commented at 2:02 PM on September 14, 2022:
member
I think it's a nice improvement. Will PR if there's any interest.
theuni
commented at 8:15 PM on September 14, 2022:
member
I too think that we don't use the variadic and it's Ok to drop it in the patch:
We use, for example, the DTRACE_PROBE6(context, event, a, b, c, d, e, f) macro which expands to STAP_PROBE6(provider,probe,parm1,parm2,parm3,parm4,parm5,parm6) expanding too _SDT_PROBE(provider, name, 6, (arg1, arg2, arg3, arg4, arg5, arg6)) and again expanding too (_SDT_ASM_BODY(provider, name, _SDT_ASM_ARGS, (n)) which is defined as _SDT_ASM_BODY(provider, name, pack_args, args, ...) containing the variadic. We only fill in the provider, name, pack_args, args and don't use the variadic.
The the tracepoint tests in this task succeed. Also, for reference, this also came up in #25528 (comment) and the variadic was only recently re-added in sytemtap@ecab2afe. If needed, we could also have our own sdt.h at some point with everything we don't use thrown out.
Note that not only do we do not use the variadic, it's not even hooked up to anything. _SDT_ASM_BODY doesn't use __VA_ARGS__ at all, so any extra args passed in would just be discarded anyway. I'm assuming this is a placeholder for some later upstream feature (or maybe something someone forgot to remove), but for now it's doing nothing but triggering a warning.
DrahtBot
commented at 11:26 PM on September 14, 2022:
contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
No conflicts as of last run.
maflcko
commented at 6:14 AM on September 15, 2022:
member
Here's a slightly more invasive change that eliminates the possibility of undefined behavior
Strictly speaking, this will keep the UB. Recall that an enum class is allowed to hold any integral value as long as it fits in the underlying type (even if the value is not named in the enum). So you'd have to re-add the assert to avoid UB (an uninitialized value), in which case the warning would likely re-appear.
theuni
commented at 1:12 PM on September 15, 2022:
member
Here's a slightly more invasive change that eliminates the possibility of undefined behavior
Strictly speaking, this will keep the UB. Recall that an enum class is allowed to hold any integral value as long as it fits in the underlying type (even if the value is not named in the enum). So you'd have to re-add the assert to avoid UB (an uninitialized value), in which case the warning would likely re-appear.
Heh, I suppose I invited the pedantry there.
Correct, it wouldn't eliminate UB potential entirely here, but it would put it on par with every other switch/case over an fully-handled enum class without a default in our codebase. I don't think you're suggesting adding default: assert(0) for the others? :)
As I see it it'd be strictly an improvement (though admittedly not bulletproof) as the intent is better communicated to callers, and would cleanly eliminate this warning. Would you NACK it as a PR?
fanquake referenced this in commit f332c4f64d on Sep 15, 2022
maflcko
commented at 2:36 PM on September 15, 2022:
member
I don't think you're suggesting adding default: assert(0) for the others? :)
sidhujag referenced this in commit 1c746cca25 on Sep 15, 2022
fanquake referenced this in commit 44a29758a0 on Oct 4, 2022
fanquake force-pushed on Oct 4, 2022
sidhujag referenced this in commit 73e27f2fe2 on Oct 4, 2022
mruddy
commented at 1:44 AM on November 1, 2022:
contributor
Concept ACK on enabling the warning messages when CXXFLAGS is set.
These changes would benefit what I was working on in #26393.
I'd probably support defaulting --enable-suppress-external-warnings, but that can be done later/separate.
I rebased the current 6 commits onto the current master, 43e813cab266eef42e622519836f171f6a18d426, and did a build with depends using:
CONFIG_SITE="$PWD/depends/x86_64-pc-linux-gnu/share/config.site" ./configure --enable-debug CXXFLAGS="-O0"
and got a ton of boost warnings related to -Wsuggest-override.
I went back and did another build using:
CONFIG_SITE="$PWD/depends/x86_64-pc-linux-gnu/share/config.site" ./configure --enable-suppress-external-warnings --enable-debug CXXFLAGS="-O0"
and got a clean build.
fanquake referenced this in commit e1fb7381be on Dec 10, 2022
hebasto
commented at 3:38 PM on December 10, 2022:
member
assert(0) doesn't prevent this code from being hit in -DNDEBUG builds, so I believe this warning is correct.
There is also Assert(0), which shouldn't have this static analysis problem.
Edit: Ok, maybe it does. Though annotating assertion_fail with [[noreturn]] might fix that.
Assert(0) combined with [[noreturn]] assertion_fail does fix the warning.
maflcko
commented at 12:38 PM on December 12, 2022:
Not sure if we want to maim code to please broken and ancient compilers
hebasto
commented at 10:12 AM on January 24, 2023:
This only change is better than disabling a useful warning, isn't it?
maflcko
commented at 10:20 AM on January 24, 2023:
No. C++ allows for uninitialized values, and allows for valgrind to find code logic errors based on this.
I fail to see how using a warning emitted by a known-broken compiler is an excuse to worsen the code.
Either the compiler shouldn't be used or the warning be disabled on the compiler.
maflcko
commented at 12:01 PM on February 1, 2023:
Looks like this (and other changes) can be dropped after #27013
bitcoin deleted a comment on Jan 23, 2023
DrahtBot added the label Needs rebase on Jan 24, 2023
fanquake force-pushed on Jan 24, 2023
maflcko dismissed
maflcko
commented at 10:20 AM on January 24, 2023:
member
idk. Probably NACK?
DrahtBot removed the label Needs rebase on Jan 24, 2023
fanquake
commented at 11:32 AM on January 24, 2023:
member
idk. Probably NACK?
Are you NACKING the whole (concept of the) pull, or the single fuzz change? I agree some of these changes are not ideal, and I would rather avoid the use of old/broken compilers entirely. I'll continue to split out changes where they make sense, and look at fixing issues in another way.
The main issue here is the use of old/broken compilers in the CI, in combination with -Werror, as we're essentially converting incorrect output from old/broken compilers, into progress blocking issues, as the CI will be red.
maflcko
commented at 11:49 AM on January 24, 2023:
member
In the OP you mention that it is possible to pass "the relevant -Wno- option in CXXFLAGS." So I wonder why you opted to change the code (https://github.com/bitcoin/bitcoin/pull/25972#discussion_r1045780114) instead of using your suggestion to simply disable the broken warning?
The main issue here is the use of old/broken compilers in the CI
I presume the reason we are using those compilers is, that they are the versions used when cross-compiling to Windows (by the average user, or by our guix build). So unless the average user, or at least the guix build is using a different version, we are stuck with them.
fanquake
commented at 11:57 AM on January 24, 2023:
member
In the OP you mention that it is possible to pass "the relevant -Wno- option
If you're happy for us to maintain various -Wno options throughout the CI configs, to suppress warnings from the broken compilers, I'm happy to convert to that approach. I'd say, in general, if we can make a single line code change, vs adding further configuration complexitiy to our CI, that would seem preferred? In the op I was also generally referring to 3rd party code, as that is what is packaged in depends, and is usually, the primary cause of copious amounts of warnings.
I presume the reason we are using those compilers is,
I'm not arguing against testing the older compilers, I'm saying I don't think we should be testing them with -Werror, when we already know they are broken, going to produce garbage output, and cause pointless build failures.
maflcko
commented at 12:05 PM on January 24, 2023:
member
a single line code change, vs adding further configuration complexitiy to our CI, that would seem preferred?
I don't think a single line code change is enough? The CI currently fails with:
script/interpreter.cpp:1530:42: error: ‘ext_flag’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1530 | const uint8_t spend_type = (ext_flag << 1) + (have_annex ? 1 : 0); // The low bit indicates whether an annex is present.
When the options are to refactor consensus code, potentially introducing a consensus bug, or disabling a harmless, but broken compiler warning, I think the answer should be clear? This is also the approach we take with sanitizer suppressions.
fanquake
commented at 12:07 PM on January 24, 2023:
member
I don't think a single line code change is enough? The CI currently fails with:
A single line change was enough to fix all other warnings. The one you are referencing, clearly not, which is why the fix was split off into #26101 for further discussion. The other issue there is that we probably don't want to turn off -Wmaybe-uninitialized warnings, for GCC 10, which is currently our release compiler.
maflcko
commented at 12:48 PM on January 24, 2023:
member
In any case, if this is currently not for merge (CI is red) and depending on another change, maybe close or mark as draft for now?
fanquake marked this as a draft on Jan 24, 2023
fanquake
commented at 1:18 PM on January 24, 2023:
member
maybe close or mark as draft for now?
Drafted. This is based on #26945 and may use #26101. May also change the approach to suppressions instead.
fanquake referenced this in commit eee2c28985 on Jan 26, 2023
DrahtBot added the label Needs rebase on Jan 26, 2023
fanquake force-pushed on Jan 26, 2023
fanquake
commented at 1:58 PM on January 26, 2023:
member
Rebased for #26945 and rebased on top of #26101 for now/CI. However note that this is now failing with what is likely another false positive, in some recently introduced code:
test/fuzz/partially_downloaded_block.cpp: In function ‘void partially_downloaded_block_fuzz_target(FuzzBufferType)’:
test/fuzz/partially_downloaded_block.cpp:122:46: error: ‘iftmp.71’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
pdb.m_check_block_mock = FuzzedCheckBlock(
~~~~~~~~~~~~~~~~^
fail_check_block ?
~~~~~~~~~~~~~~~~~~
std::optional<BlockValidationResult>{validation_result} :
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
std::nullopt);
~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:16735: test/fuzz/fuzz-partially_downloaded_block.o] Error 1
It's clear that using -Werror with older/broken compilers is going to be impossible to maintain, and trying to suppress individual classes of warnings is a constantly moving target. Will PR some changes to improve this.
maflcko
commented at 2:39 PM on January 26, 2023:
member
Right, so the previous patch just happened to be a single-line fixup. Though, for newly written code you'll harass devs with incorrect errors and force them to regress their code. So I'll remain Approach NACK on the current state of this pull.
fanquake
commented at 2:54 PM on January 26, 2023:
member
you'll harass devs with incorrect errors and force them to regress their code
No, the (global) use of -Werror with old, broken compilers, in the CI, would be harassing them. Which is what I alluded to fixing in my previous comment. Warnings in our code when built with depends (relases), have always existed, but have either been suppressed, or ignored entirely, by not actually turning on any warning flags. The point of this pull is to turn on the flags with depends (release) builds, so that you actually get (useful/meaningful) output. This is now possible given there are less warnings produced by the depends libs in general, and we have the infra in our build system to easily suppress warnings that are not coming from our own code.
Given the same source code, and two versions of the same compiler, I don't see why we have decided that turning the CI red, when the older (known broken) compiler spits out a false-positive warning, is something we need to deal with. If devs/users want to compile their code with an older compiler, that's fine, and we'll continue to do our best to support that, but there is no-reason for that older compiler, producing an incorrect warning, to be some sort of blocker in our CI.
DrahtBot removed the label Needs rebase on Jan 26, 2023
sidhujag referenced this in commit 53564628e1 on Jan 26, 2023
fanquake force-pushed on Feb 1, 2023
fanquake
commented at 3:28 PM on February 1, 2023:
member
Dropped some changes out of here, and rebased on top of #27013.
in
src/script/interpreter.cpp:1495
in
4242db9259outdated
1493 | // An upgradable public key version (with a size not 32-byte) may
1494 | // request a different key_version with a new sigversion.
1495 | key_version = 0;
1496 | break;
1497 | - default:1498 | - assert(false);
DrahtBot added the label CI failed on Apr 20, 2023
fanquake force-pushed on May 2, 2023
fanquake force-pushed on May 17, 2023
fanquake force-pushed on Jun 2, 2023
fanquake force-pushed on Jun 15, 2023
fanquake force-pushed on Jun 15, 2023
maflcko
commented at 4:24 PM on June 15, 2023:
member
Can you bump the g++-mingw-w64-x86-64-posix task to debian:bookworm to see what happens?
fanquake force-pushed on Jun 16, 2023
fanquake
commented at 8:19 AM on June 16, 2023:
member
Can you bump the g++-mingw-w64-x86-64-posix task to debian:bookworm to see what happens?
Added a commit. Also rebased, and fixed up the commit messages after #27872.
maflcko
commented at 8:50 AM on June 16, 2023:
member
The error is still there, which means you'll have to set Wno for -Werror=maybe-uninitialized for g++-mingw-w64-x86-64-posix?
fanquake force-pushed on Jun 17, 2023
maflcko
commented at 7:38 AM on June 19, 2023:
member
Looks like there are others now:
leveldb/db/db_impl.cc:740:28: error: too many arguments for format [-Werror=format-extra-args]
740 | Log(options_.info_log, "Moved #%lld to level-%d %lld bytes %s: %s\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
leveldb/db/db_impl.cc: In member function ‘leveldb::Status leveldb::DBImpl::FinishCompactionOutputFile(leveldb::DBImpl::CompactionState*, leveldb::Iterator*)’:
leveldb/db/db_impl.cc:861:50: error: unknown conversion type character ‘l’ in format [-Werror=format=]
861 | Log(options_.info_log, "Generated table #%llu@%d: %lld keys, %lld bytes",
| ^
leveldb/db/db_impl.cc:861:54: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘uint64_t’ {aka ‘long long unsigned int’} [-Werror=format=]
861 | Log(options_.info_log, "Generated table #%llu@%d: %lld keys, %lld bytes",
| ~^
| |
| int
| %I64d
862 | (unsigned long long)output_number, compact->compaction->level(),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| uint64_t {aka long long unsigned int}
leveldb/db/db_impl.cc:861:59: error: unknown conversion type character ‘l’ in format [-Werror=format=]
861 | Log(options_.info_log, "Generated table #%llu@%d: %lld keys, %lld bytes",
| ^
leveldb/db/db_impl.cc:861:70: error: unknown conversion type character ‘l’ in format [-Werror=format=]
861 | Log(options_.info_log, "Generated table #%llu@%d: %lld keys, %lld bytes",
| ^
leveldb/db/db_impl.cc:861:30: error: too many arguments for format [-Werror=format-extra-args]
861 | Log(options_.info_log, "Generated table #%llu@%d: %lld keys, %lld bytes",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
leveldb/db/db_impl.cc: In member function ‘leveldb::Status leveldb::DBImpl::InstallCompactionResults(leveldb::DBImpl::CompactionState*)’:
leveldb/db/db_impl.cc:872:62: error: unknown conversion type character ‘l’ in format [-Werror=format=]
872 | Log(options_.info_log, "Compacted %d@%d + %d@%d files => %lld bytes",
| ^
leveldb/db/db_impl.cc:872:26: error: too many arguments for format [-Werror=format-extra-args]
872 | Log(options_.info_log, "Compacted %d@%d + %d@%d files => %lld bytes",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
Those will be fixed in debian:bookworm, though there are others popping up as well ...
fanquake force-pushed on Jun 28, 2023
fanquake
commented at 11:14 AM on June 28, 2023:
member
though there are others popping up as well ...
I think at least for the (windows) ones coming out of leveldb, we could filter them out, as we do with other warnings there.
DrahtBot added the label Needs rebase on Jul 27, 2023
fanquake force-pushed on Jul 27, 2023
DrahtBot removed the label Needs rebase on Jul 27, 2023
glozow referenced this in commit 7c66a4b610 on Aug 3, 2023
fanquake force-pushed on Aug 3, 2023
fanquake force-pushed on Aug 7, 2023
fanquake force-pushed on Aug 23, 2023
maflcko
commented at 8:19 AM on August 24, 2023:
member
Why not disable this for the windows build temporarily? #25972 (comment)
fanquake
commented at 3:37 PM on August 30, 2023:
member
Why not disable this for the windows build temporarily? #25972 (comment)
I'm going to put this ontop of the GCC 12 commits and work from there, given that's the direction we're going.
maflcko
commented at 5:26 PM on September 20, 2023:
Not sure
Is this even still needed?
fanquake
commented at 2:10 PM on September 21, 2023:
The changes here will fail without it:
torcontrol.cpp: In static member function ‘static void TorControlConnection::readcb(bufferevent*, void*)’:
torcontrol.cpp:94:28: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
94 | self->message.code = ToIntegral<int>(s.substr(0, 3)).value_or(0);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./netaddress.h:18,
from ./torcontrol.h:11,
from torcontrol.cpp:6:
./util/strencodings.h:184:7: note: ‘result’ was declared here
184 | T result;
| ^~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:11088: libbitcoin_node_a-torcontrol.o] Error 1
make[2]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-w64-mingw32/src'
make[1]: *** [Makefile:20110: all-recursive] Error 1
make[1]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-w64-mingw32/src'
make: *** [Makefile:814: all-recursive] Error 1
maflcko
commented at 2:15 PM on September 21, 2023:
Seems like your change may be fine here, but I think generally it will cause issues if this pull is going to be merged.
Imagine another place that needs error checking for exactly int. What are they supposed to do?
Seems better to include the workaround here, than to transform it into a blocker for another patch.
fanquake
commented at 9:32 AM on September 26, 2023:
Switched to -Wno-error=maybe-uninitialized
hebasto referenced this in commit 2e1d4bf444 on Sep 22, 2023
Frank-GER referenced this in commit 5ca8948475 on Sep 25, 2023
fanquake force-pushed on Sep 26, 2023
fanquake
commented at 9:33 AM on September 26, 2023:
member
maflcko
commented at 9:35 AM on September 26, 2023:
member
lgtm
fanquake marked this as ready for review on Sep 26, 2023
sidhujag referenced this in commit 109e98bd01 on Sep 26, 2023
in
ci/test/00_setup_env_win64.sh:10
in
ef94d73804outdated
6 | @@ -7,13 +7,10 @@
7 | export LC_ALL=C.UTF-8
8 |
9 | export CONTAINER_NAME=ci_win64
10 | -export CI_IMAGE_NAME_TAG="docker.io/amd64/ubuntu:22.04" # Check that Jammy can cross-compile to win64 11 | +export CI_IMAGE_NAME_TAG="docker.io/amd64/ubuntu:23.10" # Check that https://packages.ubuntu.com/mantic/g++-mingw-w64-x86-64-posix (version 12.2, similar to guix) can cross-compile
maflcko
commented at 12:30 PM on September 29, 2023:
Just for the order in which to merge things, I'd prefer to keep this in sync with guix, so merging "https://github.com/bitcoin/bitcoin/pull/27897 (guix: use GCC 12.3.0 to build releases by fanquake)" first would be better.
Also, it would be good to stay on some LTS release to avoid having to touch CI every few months.
Maybe use the 24.04 tag, once it is available in a month or two?
fanquake
commented at 10:41 AM on October 2, 2023:
Yea. Rebased this back on #27897, and moved this to draft for now.
fanquake force-pushed on Oct 2, 2023
fanquake marked this as a draft on Oct 2, 2023
Retropex referenced this in commit 32fc68d9b1 on Oct 4, 2023
fanquake force-pushed on Oct 4, 2023
fanquake force-pushed on Oct 13, 2023
fanquake force-pushed on Oct 30, 2023
fanquake force-pushed on Nov 3, 2023
fanquake force-pushed on Nov 8, 2023
DrahtBot added the label Needs rebase on Nov 13, 2023
fanquake force-pushed on Nov 13, 2023
DrahtBot removed the label Needs rebase on Nov 13, 2023
fanquake force-pushed on Nov 16, 2023
fanquake force-pushed on Nov 22, 2023
fanquake force-pushed on Nov 28, 2023
div72 referenced this in commit 5c57147426 on Dec 3, 2023
fanquake force-pushed on Dec 7, 2023
DrahtBot added the label CI failed on Dec 7, 2023
fanquake force-pushed on Dec 8, 2023
maflcko
commented at 2:46 PM on December 8, 2023:
member
Will add it's also a better match to Guix, as there we are using GCC 12.3.0 & 11.x headers, and that is what would be used on Mantic, vs GCC 12.2.0 and 10.x on Debian.
Is there a particular reason it can't be used in the interim, so we have a better matching CI and less workarounds to maintain? I assume we are going to switch to Noble when it releases in April, so Mantic going EOL in July isn't an issue?
Going to drop this for now, we can retain the mingw-w64 header workaround, and the smaller mismatch.
fanquake force-pushed on Mar 14, 2024
fanquake
commented at 4:01 PM on March 14, 2024:
member
Dropped the Mantic bump, rebased on top of #29165, which should fix the failing multiprocess CI (Clang bump to 15), and added a commit to fix the tidy job (works around a bug in llvm 17, fixed in 18).
fanquake
commented at 11:05 AM on March 15, 2024:
member
DrahtBot removed the label DrahtBot Guix build requested on Mar 19, 2024
fanquake force-pushed on Mar 20, 2024
fanquake force-pushed on Mar 26, 2024
fanquake force-pushed on Apr 1, 2024
DrahtBot added the label CI failed on Apr 1, 2024
janus referenced this in commit 7bfb1b86db on Apr 1, 2024
DrahtBot removed the label CI failed on Apr 5, 2024
fanquake force-pushed on Apr 6, 2024
hebasto
commented at 5:48 PM on April 6, 2024:
member
Tested 8d2622f96f7bd25a4ab9c667bd00f374cfcc15a6.
The CI script ci/test/00_setup_env_native_valgrind.sh fail locally:
wallet/walletdb.cpp:1431:15: error: code will never be executed [-Werror,-Wunreachable-code]
error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)));
^
wallet/walletdb.cpp:1426:19: note: silence by adding parentheses to mark code as explicitly dead
if constexpr (true) {
^
/* DISABLES CODE */ ( )
wallet/walletdb.cpp:1419:19: error: code will never be executed [-Werror,-Wunreachable-code]
error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path)));
^
wallet/walletdb.cpp:1414:23: note: silence by adding parentheses to mark code as explicitly dead
if constexpr (true) {
^
/* DISABLES CODE */ ( )
2 errors generated.
fanquake
commented at 5:58 PM on April 6, 2024:
member
The CI script ci/test/00_setup_env_native_valgrind.sh fail locally:
That is a bug in Clang 14. Fixed in 15.
fanquake force-pushed on Apr 10, 2024
DrahtBot added the label CI failed on Apr 10, 2024
fanquake force-pushed on Apr 14, 2024
DrahtBot removed the label CI failed on Apr 14, 2024
maflcko added the label DrahtBot Guix build requested on Apr 23, 2024
DrahtBot
commented at 4:36 PM on April 23, 2024:
contributor
DrahtBot removed the label DrahtBot Guix build requested on Apr 23, 2024
fanquake force-pushed on Apr 25, 2024
DrahtBot added the label CI failed on Apr 26, 2024
DrahtBot
commented at 1:06 AM on April 26, 2024:
contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot removed the label CI failed on Apr 26, 2024
fanquake force-pushed on Apr 29, 2024
theuni
commented at 6:22 PM on May 1, 2024:
member
What's the status of this?
fanquake
commented at 1:18 AM on May 2, 2024:
member
It's waiting for the PR linked in the description. Otherwise, should be mergable.
maflcko added the label Needs CMake port on May 2, 2024
fanquake force-pushed on May 2, 2024
fanquake
commented at 1:26 PM on May 2, 2024:
member
This will likely fail one last time, but should be good after #30012.
fanquake force-pushed on May 2, 2024
DrahtBot added the label CI failed on May 2, 2024
DrahtBot
commented at 2:05 PM on May 2, 2024:
contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
fanquake marked this as ready for review on May 2, 2024
DrahtBot removed the label CI failed on May 2, 2024
ci: disable -Werror=maybe-uninitialized for Windows builds
This produces false positives, such as:
```bash
torcontrol.cpp: In static member function ‘static void TorControlConnection::readcb(bufferevent*, void*)’:
torcontrol.cpp:94:28: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
94 | self->message.code = ToIntegral<int>(s.substr(0, 3)).value_or(0);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./netaddress.h:18,
from ./torcontrol.h:11,
from torcontrol.cpp:6:
./util/strencodings.h:184:7: note: ‘result’ was declared here
184 | T result;
| ^~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:11088: libbitcoin_node_a-torcontrol.o] Error 1
```
7469ac7032
ci: remove -Warray-bounds from -Werror for win64
These warnings are coming from leveldb, and appear to be fixed starting
with GCC 13.
bec6a88fbc
ci: remove -Wdocumentation from -Werror in multiprocess CI
The issues here are coming from Boost Test code.
b088062e68
build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set
Now that CXXFLAGS are back in user control, I don't think there's a
reason to no-longer use our warning flags when CXXFLAGS has been
overriden (this includes when building from depends).
Anyone can suppress warnings from third-party code by
passing the relevant `-Wno-` options in CXXFLAGS.
Fixes: #18092.
f0e22be69a
fanquake force-pushed on May 3, 2024
fanquake added the label DrahtBot Guix build requested on May 3, 2024
maflcko
commented at 9:39 AM on May 3, 2024:
member
Would it make sense to mention that -Wno-error=... should be passed, when needed, in the compile docs?
theuni
commented at 1:32 PM on May 3, 2024:
member
I can't repro the -Wmaybe-uninitialized issue. Might it be possible to just add the initializations (even if they're unnecessary) rather than disabling?
Like for the example provided, making it:
T result{};
Edit: nm, I see there are other false-positives as well.
theuni approved
theuni
commented at 1:38 PM on May 3, 2024:
member
ACKf0e22be69a15248c42964d57f44ce8c37a36081d. It'll be nice to have this fixed.
maflcko
commented at 1:43 PM on May 3, 2024:
member
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-04-15 00:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me