Skip to content
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

depends: remove FORCE_USE_SYSTEM_CLANG & native_llvm #29188

Closed
wants to merge 2 commits into from

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jan 5, 2024

Always use the system Clang when compiling for macOS.
Removes the native_llvm package.
Simplifies #21778 (which should also fix the RISC-V macOS cross build).

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 5, 2024

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
Concept ACK hebasto, dongcarl

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:

  • #25573 ([POC] guix: produce a fully -static-pie bitcoind by fanquake)
  • #25391 (guix: Use LTO to build releases by fanquake)
  • #21778 (build: LLD based macOS toolchain 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.

@hebasto
Copy link
Member

hebasto commented Jan 5, 2024

Concept ACK.

@maflcko
Copy link
Member

maflcko commented Jan 5, 2024

which should also fix the RISC-V macOS cross build

I think that is currently failing due to a bug in the llvm upstream build system (#28880 (comment))

Other than that:

Nice!

@fanquake
Copy link
Member Author

fanquake commented Jan 5, 2024

I think that is currently failing due to a bug in the llvm upstream build system (#28880 (comment))

Right, I was referring to this failure, #29078 (comment), where we can't compile cctools on RISC-V, because it just isn't supported.

@hebasto
Copy link
Member

hebasto commented Jan 5, 2024

Expecting that macOS cross-compiling CI job will fail and we need to reapply 05aca09.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the *__native_toolchain variables are no longer needed:

diff --git a/depends/Makefile b/depends/Makefile
index 319c3498df..8f33b7defa 100644
--- a/depends/Makefile
+++ b/depends/Makefile
@@ -186,7 +186,6 @@ all_packages = $(packages) $(native_packages)
 meta_depends = Makefile funcs.mk builders/default.mk hosts/default.mk hosts/$(host_os).mk builders/$(build_os).mk
 
 $(host_arch)_$(host_os)_native_binutils?=$($(host_os)_native_binutils)
-$(host_arch)_$(host_os)_native_toolchain?=$($(host_os)_native_toolchain)
 
 include funcs.mk
 
diff --git a/depends/builders/darwin.mk b/depends/builders/darwin.mk
index 8ed82b276d..7663a25ac3 100644
--- a/depends/builders/darwin.mk
+++ b/depends/builders/darwin.mk
@@ -22,7 +22,6 @@ darwin_NM:=$(shell xcrun -f nm)
 darwin_INSTALL_NAME_TOOL:=$(shell xcrun -f install_name_tool)
 darwin_DSYMUTIL:=$(shell xcrun -f dsymutil)
 darwin_native_binutils=
-darwin_native_toolchain=
 
 x86_64_darwin_CFLAGS += -arch x86_64
 x86_64_darwin_CXXFLAGS += -arch x86_64
diff --git a/depends/funcs.mk b/depends/funcs.mk
index 987be4e611..d1db87305f 100644
--- a/depends/funcs.mk
+++ b/depends/funcs.mk
@@ -47,7 +47,7 @@ endef
 
 define int_get_build_id
 $(eval $(1)_dependencies += $($(1)_$(host_arch)_$(host_os)_dependencies) $($(1)_$(host_os)_dependencies))
-$(eval $(1)_all_dependencies:=$(call int_get_all_dependencies,$(1),$($($(1)_type)_native_toolchain) $($($(1)_type)_native_binutils) $($(1)_dependencies)))
+$(eval $(1)_all_dependencies:=$(call int_get_all_dependencies,$(1),$($($(1)_type)_native_binutils) $($(1)_dependencies)))
 $(foreach dep,$($(1)_all_dependencies),$(eval $(1)_build_id_deps+=$(dep)-$($(dep)_version)-$($(dep)_recipe_hash)))
 $(eval $(1)_build_id_long:=$(1)-$($(1)_version)-$($(1)_recipe_hash)-$(release_type) $($(1)_build_id_deps) $($($(1)_type)_id))
 $(eval $(1)_build_id:=$(shell echo -n "$($(1)_build_id_long)" | $(build_SHA256SUM) | cut -c-$(HASH_LENGTH)))
@@ -288,5 +288,5 @@ $(foreach package,$(all_packages),$(eval $(call int_config_attach_build_config,$
 #create build targets
 $(foreach package,$(all_packages),$(eval $(call int_add_cmds,$(package))))
 
-#special exception: if a toolchain package exists, all non-native packages depend on it
-$(foreach package,$(packages),$(eval $($(package)_extracted): |$($($(host_arch)_$(host_os)_native_toolchain)_cached) $($($(host_arch)_$(host_os)_native_binutils)_cached) ))
+#special exception: if a native binutils package exists, all non-native packages depend on it
+$(foreach package,$(packages),$(eval $($(package)_extracted): |$($($(host_arch)_$(host_os)_native_binutils)_cached) ))
diff --git a/depends/hosts/darwin.mk b/depends/hosts/darwin.mk
index baaeadf49f..667793d7f6 100644
--- a/depends/hosts/darwin.mk
+++ b/depends/hosts/darwin.mk
@@ -7,7 +7,6 @@ LD64_VERSION=711
 OSX_SDK=$(SDK_PATH)/Xcode-$(XCODE_VERSION)-$(XCODE_BUILD_ID)-extracted-SDK-with-libcxx-headers
 
 darwin_native_binutils=native_cctools
-darwin_native_toolchain=
 
 # We can't just use $(shell command -v clang) because GNU Make handles builtins
 # in a special way and doesn't know that `command` is a POSIX-standard builtin

@dongcarl
Copy link
Contributor

dongcarl commented Jan 5, 2024

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 6, 2024

Guix builds (on x86_64)

File commit cb6d619
(master)
commit 3586e7a
(master and this pull)
SHA256SUMS.part 25b0f7d5dc1bab0f... 1fe4439c888aad71...
*-aarch64-linux-gnu-debug.tar.gz d9fef4b8323e3bd8... 427d26ce63b0be88...
*-aarch64-linux-gnu.tar.gz a6279d213aa5fcb0... 6384facf1e8a6bf7...
*-arm-linux-gnueabihf-debug.tar.gz d5baaff084c918d9... 6f0e123e8a3e2ec0...
*-arm-linux-gnueabihf.tar.gz c84bd57a9661f8d3... 749024714f5c4ebf...
*-arm64-apple-darwin-unsigned.tar.gz 45bc2f11ac632ef5... 455d22dc5a300d00...
*-arm64-apple-darwin-unsigned.zip 9a5dc5f5708f0196... 80e84144408bfe7a...
*-arm64-apple-darwin.tar.gz 78458fd1cd379ed8... 0f365c3d9d9c33e8...
*-powerpc64-linux-gnu-debug.tar.gz a1132cc7ce5fb373... 159707184f5a66ee...
*-powerpc64-linux-gnu.tar.gz 034b846017818664... 8194d62a198ab950...
*-powerpc64le-linux-gnu-debug.tar.gz 52d8a6e99ed1bf30... c90e474eca7a4b74...
*-powerpc64le-linux-gnu.tar.gz 20a94b83dcaa9420... a495c01f46c7e48f...
*-riscv64-linux-gnu-debug.tar.gz 3328552cb40ec1bc... d2bd86b6cc35600d...
*-riscv64-linux-gnu.tar.gz 3f3bcebe24a30f1b... 1f79d64ecfe5e65a...
*-x86_64-apple-darwin-unsigned.tar.gz facd07ab5e0373b1... 8cd25bce60eabae1...
*-x86_64-apple-darwin-unsigned.zip 762f3150305749ba... 7d01ee257fed5695...
*-x86_64-apple-darwin.tar.gz 31419dc8280d54ba... 48d9fecbf562dac0...
*-x86_64-linux-gnu-debug.tar.gz 0edbe9f6b1792ab2... 07465fccb623365e...
*-x86_64-linux-gnu.tar.gz 05b21c8dc5c80f9a... e6ff38d196e53d66...
*.tar.gz 4dd5216c26da11de... ad5b7ef3d93ae99b...
guix_build.log 0f8de4fb5b67191a... a57f8a66c5ab2d2f...
guix_build.log.diff bb1069f9be726950...

hebasto and others added 2 commits January 9, 2024 10:40
This change is required to support Clang < 17.
Always use the system Clang when compiling for macOS.
@fanquake
Copy link
Member Author

fanquake commented Jan 9, 2024

Pulled back in the commit to fix Qt for Clang < 17.

@fanquake
Copy link
Member Author

fanquake commented Jan 9, 2024

I'm just going to squash this back into #21778, and get it done together. The changes (combined with lld) are now working.

@fanquake fanquake closed this Jan 9, 2024
@fanquake fanquake deleted the depends_use_system_clang branch January 9, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants