Buildsystem: Refactor compiler detection code

[dustdfg]

• Delete old check for gcc 8 as we require 9 or higher
• Flatten branches for clang and apple clang

I don't have mac so couldn't normally check. I synthetically set relevant variables to all possible values to check if it works and it works but how it will behave it normal environment ¯\_(ツ)_/¯

[dustdfg]

I also though about turning it into one line of code vanilla = methods.is_vanilla_clang(env) because I assume it will return False if we are not at mac but I doubt...

[bruvzg]

It will return False if you are not building for macOS/iOS, the function is simply checking if clang version string have Apple in it.

[dustdfg]

I know but it feels somewhat strange and hacky. If method was called is_apple_clang which is more good name it would contain semantic of if platform == "macos" in the name. Now if you fold it to one line it looses its semantic... And I doubt that renaming it is allowed because someone who builds custom godot for macos with a big chance relies on it. If I am allowed to rename it I will gladly do it

[bruvzg]

I think it's ok to rename it, compatibility is important for documented public API, but this is an internal build system method.

[fire]

What is the benefit of deleting old gcc 8 versions? RHEL8, Alma Linux 8 and Rocky 8 still use gcc 8.

RHEL8 : gcc 8.x or gcc 9.x in app stream.

https://access.redhat.com/solutions/19458

[dustdfg]

What is the benefit of deleting old gcc 8 versions? RHEL8, Alma Linux 8 and Rocky 8 still use gcc 8.

RHEL8 : gcc 8.x or gcc 9.x in app stream.

https://access.redhat.com/solutions/19458

We already require minimum gcc 9:

godot/SConstruct

Lines 642 to 650 in 76fa7b2

	elif methods.using_gcc(env):
	if cc_version_major < 9:
	print_error(
	"Detected GCC version older than 9, which does not fully support "
	"C++17, or has bugs when compiling Godot. Supported versions are 9 "
	"and later. Use a newer GCC version, or Clang 6 or later by passing "
	'"use_llvm=yes" to the SCons command line.'
	)
	Exit(255)

So having a check if compiler is < 8 doesn't have any sense

[akien-mga]

Looks good to me.

The only risk is if the is_apple_clang method is not 100% reliable, it might then wrongly flag actual Apple clang builds as not Apple (other false negatives wouldn't be a problem). So this needs to be tested on macOS and with osxcross cross-compilation on Linux just to be sure.

[shana]

Looks good to me.

The only risk is if the is_apple_clang method is not 100% reliable, it might then wrongly flag actual Apple clang builds as not Apple (other false negatives wouldn't be a problem). So this needs to be tested on macOS and with osxcross cross-compilation on Linux just to be sure.

This could also affect console platforms that use clang toolchains and could be misidentified. I would lean towards being a bit more paranoid about using both is_apple_clang and other more specific platform checks to make sure only apple platforms really fall into that condition.

[akien-mga]

Looks good to me.
The only risk is if the is_apple_clang method is not 100% reliable, it might then wrongly flag actual Apple clang builds as not Apple (other false negatives wouldn't be a problem). So this needs to be tested on macOS and with osxcross cross-compilation on Linux just to be sure.

This could also affect console platforms that use clang toolchains and could be misidentified. I would lean towards being a bit more paranoid about using both is_apple_clang and other more specific platform checks to make sure only apple platforms really fall into that condition.

I think the risk is when using Xcode actually, because if anything goes wrong, is_apple_clang can only return False. I don't see a case where it would return a false positive (True), the only way for it to be True is if the version string starts with Apple.

If it returns False while using Xcode, then yeah it might use the wrong branches. But that would imply that the current code only works by chance - I just want it tested to confirm but I don't think it's actually broken :P

So other platforms shouldn't be affected.

BTW I just tested with osxcross, and it doesn't identify itself as "AppleClang", and its version string is the one of vanilla LLVM (despite me building the Apple fork of Clang), so it seems like this is really for Xcode's weird versioning that we need the apple_clang stuff.

$ clang --version
clang version 16.0.0 (https://github.com/tpoechtrager/osxcross ff8d100f3f026b4ffbe4ce96d8aac4ce06f1278b)

---

That being said, while probably unnecessary, I see no harm in adding a platform check in is_apple_clang for extra safety.

[dustdfg]

I just curious about why is_vanilla_clang which is just negative twin of is_apple_clang wasn't questionable?

[akien-mga]

I just curious about why is_vanilla_clang which is just negative twin of is_apple_clang wasn't questionable?

It is, but since it was only used for macOS/iOS, and it's in support of a very niche use case (far most users building for macOS/iOS do it with Xcode on macOS which isn't vanilla LLVM/Clang), it wasn't as problematic if it's misidentified.

When flipping the meaning to cover what is the expected toolchain for 99% of use cases, we need to make sure it's reliable.

[Repiteo]

Thanks!