-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: Drop duplicated compiler flags #29800
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
lgtm ACK be97652 The depends ones are not needed, because depends already passes the flags through. The |
@@ -17,7 +17,7 @@ export PACKAGES="ninja-build" | |||
# BDB generates false-positives and will be removed in future | |||
export DEP_OPTS="DEBUG=1 NO_BDB=1 NO_QT=1 CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'" | |||
export GOAL="install" | |||
export BITCOIN_CONFIG="--with-sanitizers=memory --disable-hardening --with-asm=no CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --with-sanitizers=memory
has been restored in the recent push.
be97652
to
a3485af
Compare
re-ACK a3485af |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a3485af - no-longer a change in behaviour.
On the master branch @ 0d509ba, 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.