ci: Drop duplicated compiler flags #29800

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:240403-ci-cleanup changing 3 files +3 −3

hebasto commented at 3:04 PM on April 3, 2024: member

On the master branch @ 0d509bab45d292caeaf34600e57b5928757c6005, it is easy to check the "Options used to compile and link" section in the configure script output and observe duplicated compiler flags.

This PR cleans such cases up.
DrahtBot commented at 3:05 PM on April 3, 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 maflcko, fanquake

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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #29790 ([DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP by hebasto)
- #29742 ([WIP] ci: test secp256k1 MSAN asm annotations by fanquake)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label Tests on Apr 3, 2024
maflcko commented at 3:16 PM on April 3, 2024: member

lgtm ACK be97652c07a10399c08ecb98dbbaeb33b84af774

The depends ones are not needed, because depends already passes the flags through. The -g isn't needed, because it is always set by default.
in ci/test/00_setup_env_native_msan.sh:20 in be97652c07 outdated
```
  16 | @@ -17,7 +17,7 @@ export PACKAGES="ninja-build"
  17 |  # BDB generates false-positives and will be removed in future
  18 |  export DEP_OPTS="DEBUG=1 NO_BDB=1 NO_QT=1 CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
  19 |  export GOAL="install"
  20 | -export BITCOIN_CONFIG="--with-sanitizers=memory --disable-hardening --with-asm=no CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
```
fanquake commented at 3:41 PM on April 3, 2024:

I guess going forward, we'll have to remember to never add any logic/other flags to the --with-sanitizers/cmake equivalent option, that differ from just setting -fsanitize=x? Otherwise they could possibly get missed in CI.

Removing this also now skips the link check(s) and I don't see where flags are getting added to LDFLAGS elsewhere? So this is at least a change in behaviour, some (not duplicated) flags have been dropped entirely, and I'd say somewhat less clear.

hebasto commented at 2:39 PM on April 5, 2024:

The --with-sanitizers=memory has been restored in the recent push.
DrahtBot added the label CI failed on Apr 3, 2024
DrahtBot removed the label CI failed on Apr 5, 2024
ci: Drop duplicated compiler flags a3485af67d
hebasto force-pushed on Apr 5, 2024
maflcko commented at 2:59 PM on April 5, 2024: member

re-ACK a3485af67da4949c72c45acc608f8746ed0e0848
fanquake approved
fanquake commented at 4:02 PM on April 5, 2024: member

ACK a3485af67da4949c72c45acc608f8746ed0e0848 - no-longer a change in behaviour.
fanquake merged this on Apr 5, 2024
fanquake closed this on Apr 5, 2024
hebasto deleted the branch on Apr 5, 2024
Pttn referenced this in commit a60e89a2cb on Apr 13, 2024
bitcoin locked this on Apr 5, 2025

Type	Reviewers
ACK	maflcko, fanquake

Contributors

Labels

ci: Drop duplicated compiler flags #29800

Code Coverage

Reviews

Conflicts