fanquake
commented at 6:40 am on July 1, 2021:
member
By explicitly setting a C standard version we avoid any potential for issues/differences in libraries that may come about due to C STD version, as well as avoid potentially being opted into newer code / features in libraries when compiler defaults change (i.e as of 11.0.0, Clang now defaults to gnu17 over gnu11).
This should be a no-op for our release builds, because it’s just explicitly setting the default that is already being used. However this is relevant for anyone building depends with a newer compiler.
At the same time, add CXX_STANDARD for setting our C++ standard, and use that over setting -std=c++17 for cxx packages.
Guix builds:
fanquake added the label
Build system
on Jul 1, 2021
DrahtBot
commented at 8:12 am on July 1, 2021:
member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#22552 (build: Improve depends build system robustness by hebasto)
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.
hebasto
commented at 12:12 pm on July 1, 2021:
member
Concept ACK.
This should be a no-op for our release builds…
Could be verified on by-package basis with #21995.
fanquake force-pushed
on Jul 1, 2021
laanwj
commented at 10:04 am on July 21, 2021:
member
Concept ACK.
Would it make sense to define a global e.g. EXTRA_CFLAGS variable and use it for every package, instead of repeating this for every single one?
tryphe
commented at 10:39 pm on August 1, 2021:
contributor
Concept ACK
Zero-1729
commented at 12:05 pm on August 21, 2021:
contributor
Concept ACK
Would it make sense to define a global e.g. EXTRA_CFLAGS variable and use it for every package, instead of repeating this for every single one?
Is there a reason for not going this route?
fanquake force-pushed
on Aug 31, 2021
fanquake
commented at 6:06 am on August 31, 2021:
member
I’ve reworked this, and based it on top of #22840.
Would it make sense to define a global e.g. EXTRA_CFLAGS variable and use it for every package, instead of repeating this for every single one?
I’ve added DEFAULT_C_STD and DEFAULT_CXX_STD variables to the depends makefile. Defaulting to c11 and c++17. This means the versions are set in one place, and can also be overriden if someone wants to experiment building with a different version. i.e gmake -C depends DEFAULT_CXX_STD=c++20.
fanquake deleted a comment
on Aug 31, 2021
fanquake referenced this in commit
3af495d697
on Sep 2, 2021
fanquake force-pushed
on Sep 2, 2021
sidhujag referenced this in commit
1dedbbcb8b
on Sep 2, 2021
fanquake
commented at 2:28 am on September 2, 2021:
member
Rebased now that #22840 is in. Would be good to get feedback on the new approach.
fanquake added the label
DrahtBot Guix build requested
on Sep 2, 2021
theuni
commented at 4:33 pm on September 3, 2021:
member
Concept ACK. Definitely better to be explicit.
Nit: DEFAULT_FOO implies that it’s the fallback unless something specific was requested, which is not the case here as the value can be overridden. Would prefer to drop/replace the DEFAULT_ prefix.
dongcarl
commented at 6:08 pm on September 3, 2021:
member
Concept ACK!
Wondering: instead of setting it for every package, could we do it in hosts/*.mk?
DrahtBot
commented at 2:07 pm on September 4, 2021:
member
DrahtBot removed the label
DrahtBot Guix build requested
on Sep 4, 2021
practicalswift
commented at 2:34 pm on September 4, 2021:
contributor
Concept ACK
fanquake renamed this:
build: Set -std=c11 for C dependencies in depends
build: add and use C_STANDARD and CXX_STANDARD in depends
on Sep 9, 2021
fanquake force-pushed
on Sep 9, 2021
fanquake
commented at 7:38 am on September 9, 2021:
member
Nit: DEFAULT_FOO implies that it’s the fallback unless something specific was requested, which is not the case here as the value can be overridden. Would prefer to drop/replace the DEFAULT_ prefix.
Dropped the DEFAULT prefix.
Wondering: instead of setting it for every package, could we do it in hosts/*.mk?
Reworked to set both in hosts/*.mk. Is this what you had in mind?
fanquake
commented at 9:37 am on September 9, 2021:
member
Android build failure here looks unrelated, however the macOS one seems to be an issue. Doesn’t happen when building natively, only when cross-compiling qt:
Looks like clang is invoked, and our cflags are added -O2 -std=c11, however we end up with -x c++ on the end of our invocation when running some qt feature tests? Which results in Clang barfing. Will investigate.
dongcarl
commented at 2:36 pm on October 12, 2021:
member
Does this mean that qmake tests CXX by invoking it with CFLAGS?
DrahtBot added the label
Needs rebase
on Nov 14, 2021
fanquake force-pushed
on Nov 14, 2021
fanquake force-pushed
on Nov 14, 2021
fanquake
commented at 4:46 am on November 14, 2021:
member
Rebased on master, and added a potential fix for the macOS cross-compile issue. Which is to just not append QMAKE_CFLAGS to QMAKE_CXXFLAGS in the macOS configuration.
DrahtBot removed the label
Needs rebase
on Nov 14, 2021
fanquake force-pushed
on Nov 16, 2021
fanquake force-pushed
on Nov 16, 2021
fanquake
commented at 2:14 am on November 16, 2021:
member
Updated both commits to document the new variables in the depends README.
dongcarl
commented at 7:16 pm on November 16, 2021:
member
Looking clean! A few points of discussion:
Wondering if it’d make sense to somehow export the CXX_STANDARD in config.site
DrahtBot added the label
Needs rebase
on Jan 7, 2022
fanquake force-pushed
on Jan 10, 2022
fanquake
commented at 0:53 am on January 10, 2022:
member
Wondering if it’d make sense to somehow export the CXX_STANDARD in config.site
Potentially. Although if we did export it, and then passed it through to, for example, ax_cxx_compile_stdcxx, that script doesn’t support detecting C++20 or later.
Wondering if we should be putting -std flags in CFLAGS or CC
I don’t have a particular opinion here.
DrahtBot removed the label
Needs rebase
on Jan 10, 2022
DrahtBot added the label
Needs rebase
on Feb 3, 2022
fanquake force-pushed
on Feb 4, 2022
fanquake
commented at 1:46 am on February 4, 2022:
member
Rebased. @theuni@dongcarl do either of you have any additional thoughts here? Will add new Guix hashes to PR description.
fanquake removed the label
Needs rebase
on Feb 4, 2022
DrahtBot added the label
Needs rebase
on Feb 14, 2022
fanquake force-pushed
on Feb 14, 2022
DrahtBot removed the label
Needs rebase
on Feb 14, 2022
fanquake added this to the "Blockers" column in a project
fanquake force-pushed
on Apr 9, 2022
in
depends/README.md:100
in
ab58ac49ffoutdated
95@@ -96,6 +96,8 @@ The following can be set when running make: `make FOO=bar`
96 - `BASE_CACHE`: Built packages will be placed here
97 - `SDK_PATH`: Path where SDKs can be found (used by macOS)
98 - `FALLBACK_DOWNLOAD_PATH`: If a source file can't be fetched, try here before giving up
99+- `C_STANDARD`: Set the C standard version used. Defaults to `C11`.
100+- `CXX_STANDARD`: Set the C++ standard version used. Defaults to `C++17`.
dongcarl
commented at 3:52 pm on April 20, 2022:
member
I think we added a few more hosts between my last review and this one. I’m also wondering if there’s a good way to have this in hosts/default.mk somehow to stop worrying about new hosts, though hosts/default.mk does give me the scaries whenever I think about changing it.
fanquake force-pushed
on Apr 20, 2022
DrahtBot added the label
Needs rebase
on Jun 14, 2022
fanquake force-pushed
on Jun 14, 2022
fanquake removed this from the "Blockers" column in a project
DrahtBot removed the label
Needs rebase
on Jun 14, 2022
build: add and use C_STANDARD in depends7e7b3e42fa
build: add and use CXX_STANDARD in dependsf7595f1354
fanquake force-pushed
on Jun 16, 2022
fanquake
commented at 4:29 pm on June 16, 2022:
member
Updated this for the addition of new HOSTS and the LTO merge.
I’m also wondering if there’s a good way to have this in hosts/default.mk somehow to stop worrying about new hosts,
Now that we’ve also got a “global” LTO option, it could make sense to consolidate that, plus these two options into default.mk. Maybe we could just adjust C and CXX flags when using either of the 3 options? Not sure if we want to do that here, or a follow up after further discussion.
dongcarl
commented at 5:28 pm on June 16, 2022:
member
Quite a simple change, good consistency improvement can be seen in the non-depends/hosts file diffs
laanwj
commented at 9:42 pm on June 16, 2022:
member
Code review ACKf7595f1354f4618436fdab232000dc152bff315a
Changes look correct to me. I have run a succesful build, but was unable to conclude from the logging whether every package passes the correct flag to gcc/g++ (as a lot do not show the verbose build commands).
laanwj merged this
on Jun 16, 2022
laanwj closed this
on Jun 16, 2022
fanquake deleted the branch
on Jun 16, 2022
sidhujag referenced this in commit
c20081a0e2
on Jun 17, 2022
fanquake referenced this in commit
8bcdcbb0d1
on Jun 17, 2022
fanquake referenced this in commit
f862f4a74e
on Jun 17, 2022
fanquake referenced this in commit
34869114a7
on Jun 21, 2022
sidhujag referenced this in commit
8766b174f6
on Jun 21, 2022
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: 2024-12-19 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
