build: Add -fvisibility=hidden flag for macOS host #23609
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Guix builds: |
|
Fixing these warnings is fine, they've been around forever. However concept NACK on the approach. Similar to #23612, creating (undocumented) special-cases in depends, should be a last resort, especially if it's just to suppress some warnings. It would probably be unexpected to someone using depends, that when they happen to build a certain dependency, on a particular OS, they get additional (unrelated) configure options turned on, with no explanation.
Would there be any downside to just building everything in depends, for darwin, with |
Got it. Just a note that a similar approach is already used: bitcoin/depends/config.site.in Lines 114 to 116 in e7507f3
Is printing
Building everything in depends with |
f0474eb to
0cc9fa0
Compare
|
Reworked. |
0cc9fa0 to
bd66532
Compare
|
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 react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
Yes, however always turning off reduced exports when debugging, isn't really the same as turning it on when building a certain dependency, on a single OS. One is a generic, likely expected behavior, the other is a special case that's (undocumented) existence wouldn't be clear to anyone looking at the code.
I think you've misunderstood my suggestion. I meant add |
|
|
bd66532 to
9992b85
Compare
|
Reworked:
|
This was referenced Dec 24, 2021
9992b85 to
856efb0
Compare
|
Rebased 9992b85 -> 856efb0 (pr23609.03 -> pr23609.04) due to the conflict with #23817. |
Guix builds: |
|
Guix hashes, mine match @hebasto: |
856efb0 to
047d211
Compare
|
Rebased 856efb0 -> 047d211 (pr23609.04 -> pr23609.05) on top of the merged #24348. |
Guix builds: |
|
GUIX hashes: x86: arm64: |
With all benefits of reducing exports, there are no evidences of any drawback while compiling with clang for macOS. Also, when building with depends, it is expected that different translation units are being compiled with the same visibility settings.
|
Rebased 047d211 -> 258921a (pr23609.05 -> pr23609.06). |
jarolrod
reviewed
Aug 10, 2022
There was a problem hiding this comment.
this does fix some of the spamming:
master:
make -j8 2>&1 > /dev/null | grep -o “warning” | wc -l
12100
pr:
make -j8 2>&1 > /dev/null | grep -o "warning" | wc -l
12066
but i wonder if this is worth it considering the drop, perhaps attack the spamming from a different angle? Most of the warnings are coming out of boost. Put the 12100 warning in contrast with the amount of warnings i get building on linux
linux:
make -j8 2>&1 > /dev/null | grep -o "warning" | wc -l
0
I'd say most of the warnings are emitted by a compiler, and most of them can be silenced with But this PR deals with linker warnings. |
|
Friendly ping :) |
|
Personally I don't think the warnings matter. I think if you want to avoid them, use |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On master (a213bd6) when building
bitcoin-qtwith depends for macOS, clang spams with a load of warnings like this one:The cause of such warnings is the fact that
-fvisibility=hiddenis used during building of theqtpackage by default, but not for all of other building stages. That is fixed in this PR.