build: remove usage of BOOST_NO_CXX98_FUNCTION

fanquake commented at 10:48 am on May 29, 2024: member

Supposedly this is no-longer needed (and hasn’t been ported to CMake), so remove it’s usage here. Oringinally used to suppress warnings about functionality deprecated/removed from the standard library, which was still supported by some compilers.

build: remove usage of DBOOST_NO_CXX98_FUNCTION_BASE

Supposedly this is no-longer needed (and hasn't been ported to CMake),
so remove it's usage here.

fc479352f6

DrahtBot commented at 10:48 am on May 29, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Build system on May 29, 2024

maflcko commented at 11:07 am on May 29, 2024: member

Did you test this with the minimum required boost version 1.71?

fanquake commented at 11:11 am on May 29, 2024: member

Did you test this with the minimum required boost version 1.71?

Only with 1.81.0. (depends) which still has the problematic behaviour. Used a CMake CI (which has already dropped this) with Boost 1.73.0 as a proxy for anything lower (https://github.com/hebasto/bitcoin/actions/runs/9257726144/job/25466464660).

maflcko commented at 11:23 am on May 29, 2024: member

The cmake run is using g++-11, but g++-12 is required according to https://github.com/boostorg/config/pull/430/files

fanquake commented at 11:27 am on May 29, 2024: member

@hebasto how did you test this macro is no-longer required in the CMake build (given it wasn’t ported)?

hebasto commented at 11:30 am on May 29, 2024: member

@hebasto how did you test this macro is no-longer required in the CMake build (given it wasn’t ported)?

https://github.com/hebasto/bitcoin/actions/runs/9283914497

hebasto commented at 11:33 am on May 29, 2024: member

Did you test this with the minimum required boost version 1.71?

It is 1.73 since #29066.

hebasto commented at 11:39 am on May 29, 2024: member

Concept ACK.

maflcko commented at 11:51 am on May 29, 2024: member

Looks like 880d4aaf81f3d5d7fbb915905c2e61b816a6a747 affected depends, not sure if it even reproduces outside of that?

maflcko commented at 12:05 pm on May 29, 2024: member

I could only reproduce inside of depends, which has boost 1.81, which is fixed, so LGTM, I guess. :shrug:

fanquake renamed this:
~~build: remove usage of DBOOST_NO_CXX98_FUNCTION_BASE~~
build: remove usage of BOOST_NO_CXX98_FUNCTION_BASE
on May 29, 2024

fanquake marked this as ready for review on May 29, 2024

hebasto approved

hebasto commented at 1:19 pm on May 29, 2024: member

ACK fc479352f617b33d45dcbf8b2c97b144614f1145.

maflcko commented at 1:30 pm on May 29, 2024: member

I stopped seeing the warning after commit 3b2acfcfec83a4e6e50b3f21e0810274bdb05afb

maflcko commented at 1:42 pm on May 29, 2024: member

It would be good to properly explain this, because this still happens, at least with at least

--disable-suppress-external-warnings
depends boost 1.73
g++-12 and clang-18

fanquake commented at 1:43 pm on May 29, 2024: member

It would be good to properly explain this,

The explanation is that it still happens, it never should have been incorrectly dropped from the CMake build (where it can now be re-added), and this can be closed.

fanquake closed this on May 29, 2024

fanquake deleted the branch on May 29, 2024

maflcko commented at 1:50 pm on May 29, 2024: member

Does --disable-suppress-external-warnings only work with depends? (I can’t seem to get the warning on Jammy with clang-18 outside of depends)

Should --disable-suppress-external-warnings be enabled in some CI tasks?

maflcko commented at 1:58 pm on May 29, 2024: member

If it is clear, that this can only happen inside of depends, where it is fixed after 1.81, then I think it is fine to merge. However, I am trying to figure out if this can happen outside of depends.

fanquake commented at 2:22 pm on May 29, 2024: member

Does –disable-suppress-external-warnings only work with depends?

It should not be depends specific.

Should –disable-suppress-external-warnings be enabled in some CI tasks?

That depends on what we are trying to achieve, and if we are planning on patching external libraries / code and upstreaming relevant changes etc. It’s also likely to lead to more CI “breakage” on distro version, or other changes, that may not be relevant for us.

maflcko commented at 2:25 pm on May 29, 2024: member

That depends on what we are trying to achieve

Wouldn’t it be good to know about issues, such as the one here (BOOST_NO_CXX98_FUNCTION_BASE) as early as possible? Not sure how to achieve that, other than setting --disable-suppress-external-warnings in at least some CI tasks.

fanquake commented at 2:39 pm on May 29, 2024: member

Wouldn’t it be good to know about issues, as early as possible?

I think if we are going to do that, then enabling it on a rolling / nightly distro type CI, i.e fedora rawhide, would be the most useful, otherwise, I think it’s unlikely to actually turn much up, given packages are pinned in depends, and in the distro. Note that *--suppress-external-warnings was another feature not ported to CMake, as apparently it isn’t needed; so someone would have to look at porting it again, so it’s possible to achieve this same functionality, of exposing all warnings.

maflcko commented at 2:46 pm on May 29, 2024: member

I think it’s unlikely to actually turn much up, given packages are pinned in depends

at least for the sanitizer builds, the compiler is bumped regularly, so it seems good to check those bumps when they happen.

build: remove usage of BOOST_NO_CXX98_FUNCTION_BASE #30189

Code Coverage

Reviews