SCons: Fix handling of platform-specific tools, notably mingw
#101726
+41
−34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akien-mga
commented
Jan 17, 2025
akien-mga
commented
Jan 17, 2025
SConstruct
Outdated
Comment on lines
344
to
355
| # FIXME: Tool assignment happening at this stage is a direct consequence of getting the platform logic AFTER the SCons | ||
| # environment was already been constructed. Fixing this would require a broader refactor where all options are setup | ||
| # ahead of time with native validator/converter functions. | ||
| custom_tools = [] | ||
| if env["platform"] == "android": | ||
| custom_tools += ["clang", "clang++", "as", "ar", "link"] | ||
| elif env["platform"] == "web": | ||
| custom_tools += ["cc", "c++", "ar", "link", "textfile", "zip"] | ||
| elif env["platform"] == "windows" and methods.get_cmdline_bool("use_mingw", False): | ||
| custom_tools += ["mingw"] | ||
| for tool in custom_tools: | ||
| env.Tool(tool) |
There was a problem hiding this comment.
I moved this further down because there's more options overrides below, notably platform flags, and we likely want to respect them before calling detect.get_tools(env) so that this can make full use of the configuration so far.
…egister custom tools This helps move this logic out of SConstruct, keeping platforms more self contained, and helping thirdparty platforms define their own custom tools. This logic was also unreliable (the `use_mingw` one would only work if passed manually on the command line, not in e.g. `get_flags`).
akien-mga
force-pushed
the
scons-platform-custom-tools-v2
branch
from
832383d to
ed96da7
Compare
akien-mga
commented
Jan 17, 2025
Comment on lines
+422
to
+424
| tmppath = "./platform/" + env["platform"] | ||
| sys.path.insert(0, tmppath) | ||
| import detect |
There was a problem hiding this comment.
This had to move up, otherwise detect here was actually the one of the last platform we checked when parsing all of them... i.e. windows, so Linux and macOS were getting mingw registered and ending with .exe binary extensions :p
Repiteo
reviewed
Jan 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Redo of #99762 on top of #101715, aiming to fix various issues we have with the fact that we need to know the selected platform to define the right SCons "tools" to enable when creating the environment, but we need the environment to register command line options that help set said platform.
Using two environments with
env.Clone()seems to help solve the problems in #101715, and fixing the detection of whenmingwshould be set as tool via #99762 seems to make it work out of the box for Linux+mingw at least.