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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Eoin-ONeill-Yokai
Copy link

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 Eoin-ONeill-Yokai requested a review from a team as a code owner February 2, 2024 04:25
@RandomShaper
Copy link
Member

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.

@dsnopek dsnopek added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Feb 2, 2024
@dsnopek dsnopek added this to the 4.x milestone Feb 2, 2024
@Eoin-ONeill-Yokai
Copy link
Author

Eoin-ONeill-Yokai commented Feb 2, 2024

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.

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.

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

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

This change seems fine to me. However, the code in question is copied from Godot's build system. I'd prefer not to stray too far from it, since we regularly copy-paste code from there to sync up with it.

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?

If it'd still allow you to accomplish the same goal, I think I'd prefer going with that PR. It moves the code around, but we still end up with basically the same code that was copy-pasted from Godot.

@@ -2,6 +2,7 @@
import subprocess
import sys
from SCons.Script import ARGUMENTS
from SCons.Tool import Tool
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like this import is used?

@Eoin-ONeill-Yokai
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants