Skip to content

Conversation

@OzelotVanilla
Copy link
Contributor

@OzelotVanilla OzelotVanilla commented Jun 6, 2025

Related to #107199.

Solves the compilation failure problem by adding version-detect logic to check if current compiler can enable -fmodule flag.

This PR is not considered fixing the root cause of the #107199 currently.


Edit:
Currently, only clang and msvc is contained in the detecting logic, so more discussion and feedback from #107199 are needed.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Looks overcomplicated for no reason, it won't be usable for iOS/visionOS and for macOS we can simply do:

diff --git a/drivers/metal/SCsub b/drivers/metal/SCsub
index a4c1c65b82..68e281b9bb 100644
--- a/drivers/metal/SCsub
+++ b/drivers/metal/SCsub
@@ -1,5 +1,6 @@
 #!/usr/bin/env python
 from misc.utility.scons_hints import *
+from methods import is_apple_clang

 Import("env")

@@ -40,7 +41,8 @@ if "-std=gnu++17" in env_metal["CXXFLAGS"]:
 env_metal.Append(CXXFLAGS=["-std=c++20"])

 # Enable module support
-env_metal.Append(CCFLAGS=["-fmodules", "-fcxx-modules"])
+if env["platform"] != "macos" or is_apple_clang(env):
+    env_metal.Append(CCFLAGS=["-fmodules", "-fcxx-modules"])

 # Driver source files

@OzelotVanilla
Copy link
Contributor Author

I do not know if there are need to test other compilers such as gcc. If no, let me change it again. 😀

@stuartcarnie
Copy link
Contributor

I have also just created build containers that work fine with these flags on Linux per this PR, so I am not sure why these need to be changed. I don't believe the root cause is these flags.

@OzelotVanilla
Copy link
Contributor Author

OzelotVanilla commented Jun 8, 2025

I have also just created build containers that work fine with these flags on Linux per this PR, so I am not sure why these need to be changed. I don't believe the root cause is these flags.

I also agree. However since I am still new to these, I am quite not sure if this just Homebrew's clang causing the problem or not.


Edit:

I would rather call this PR a fix for compability, but not fixing the root cause 🧐.

@bruvzg
Copy link
Member

bruvzg commented Jun 8, 2025

What's the point of using upstream clang instead of Apple-clang in a first place? Installing and using Homebrew require installing Xcode command line tools, so you already have Apple-clang installed anyway.

@OzelotVanilla
Copy link
Contributor Author

What's the point of using upstream clang instead of Apple-clang in a first place? Installing and using Homebrew require installing Xcode command line tools, so you already have Apple-clang installed anyway.

(Some copied from #107199 (comment))

Since it is off-topic, so I would say for personal use.
However, from the documentation page, no requirements for compiler (type or version) is written in the requirement, so for me I believe somehow that, the latest version of compiler is considered acceptable when compiling. At least works for Vulkan I installed from brew, it looks fine.

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.

4 participants