godot-cpp icon indicating copy to clipboard operation
godot-cpp copied to clipboard

Adjust clang version checking to account for other non-vanilla clangs

Open Eoin-ONeill-Yokai opened this issue 1 year ago • 3 comments

We can no longer make the assumption that all non-vanilla clang versions are Apple's clang or that all other non vanilla clangs will work with the given arguments. There are times where additional tools might use non vanilla clang and the presumed arguments here will break compilation on those platforms. So we should ask explicitly for whether it's Apple clang or if it is vanilla clang (begins with standard "clang version xx.x.x-xxxx").

We might want to consider adding tooling-specific optimization functions long term to fetch desired optimizations on a platform basis.

I could use someone to test this on Apple platforms (ios/macos) and additionally linux to make sure that this doesn't cause a regression. It worked well for my Windows machine and my other targeted platforms.

Eoin-ONeill-Yokai avatar Feb 02 '24 04:02 Eoin-ONeill-Yokai

I guess this could be refactored so the version checking function returns the line and delegates the parsing to the caller. That way, the process would not be spawned per check.

Also, you may want to keep a catch-all else that either errors out due to not knowing what to do, or either assumes vanilla clang.

RandomShaper avatar Feb 02 '24 06:02 RandomShaper

One thing I don't really understand about the code flow of this:

Why are linker optimizations offloaded to the targets.py generate in the first place? In my custom tool for the target platform, I basically include the linker flags necessary for optimization already inside platform.py and it seems to work well as it is. This also gives me access to specific linker flags that don't exist in all clang versions. Is it simply to reduce code repetition between similar platforms that might be able to share information?

I wouldn't be opposed to erroring out in case that the version is unknown but I also think (at least with my experience with the hell that is parsing and checking ffmpeg features) that we should consider perhaps just allowing custom linker flags for optimization to be passed by argument (something like linker_optimization="") and if said arguments are blank we just skip the task entirely.

I don't know, let me know your thoughts here. What I definitely don't want to do is increase the burden of compiling for more standard platforms.

Eoin-ONeill-Yokai avatar Feb 02 '24 21:02 Eoin-ONeill-Yokai

Would this change still be necessary after PR #1392? It seems like no, because a custom platform could just not call default_flags.generate(env), right?

You're correct. Fabio and I talked about this and his changes would supersede mine. If they get approval, I will close this PR.

Eoin-ONeill-Yokai avatar Feb 16 '24 21:02 Eoin-ONeill-Yokai