SCons: Fix platform tool integration #101715
Conversation
| custom_tools = ["default"] | ||
|
|
||
| platform_arg = ARGUMENTS.get("platform", ARGUMENTS.get("p", False)) | ||
|
|
||
| if platform_arg == "android": | ||
| custom_tools = ["clang", "clang++", "as", "ar", "link"] | ||
| elif platform_arg == "web": | ||
| # Use generic POSIX build toolchain for Emscripten. | ||
| custom_tools = ["cc", "c++", "ar", "link", "textfile", "zip"] | ||
| elif os.name == "nt" and methods.get_cmdline_bool("use_mingw", False): | ||
| custom_tools = ["mingw"] |
There was a problem hiding this comment.
Changing the order of this stuff is pretty risky and regression prone. We've been bitten by this multiple times, we'd need to be extremely cautious here. That's sadly too risky for a bugfix PR IMO, we should go back to the previous working stage if that's the only option.
There was a problem hiding this comment.
Could you investigate instead whether #99762 can be used as a base to solve the issue in a better way? I'm more confident in that change, it's just a draft because I wanted to do more testing.
There was a problem hiding this comment.
I suppose I could turn this on its head by making the platform evaluation happen earlier instead of the tool assignment. That's certainly more isolated, so it wouldn't have that same risk of regression. Gonna give that a shot
That PR could be a base, but at the same time I think a lot of the platform/module specific setup should be reimplemented as SCons-native tools. Gonna have to whip up a proposal at some point that consolidates a lot of these ideas for further discussion
There was a problem hiding this comment.
In second thought I think there's merit in doing this change indeed, the current logic that relies on querying ARGUMENTS to know the selected platform before having env["platform"] is very brittle.
I'd like to combine this PR and #99762 somehow and see if we can make it safe.
There was a problem hiding this comment.
I agree there's merit to it, but now I'm not sold on it being worth digging into in isolation. There's plenty of areas here that use ARGUMENTS incorrectly, and trying to address all of them would go out of scope
| # 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.
What's the actual consequence on the SCons environment of late-registering tools like this?
There was a problem hiding this comment.
I'm not 100% certain, which is why I'm now a bit more hesitant. But my testing has made it seem like it's functionally identical, at least this early on in the environment's lifetime.
There was a problem hiding this comment.
As I feared:
$ scons p=windows
/home/akien/.local/bin/scons p=windows 2>&1 | tee -a build.log
scons: Reading SConscript files ...
Auto-detected 16 CPU cores available for build parallelism. Using 15 cores by default. You can override it with the `-j` or `num_jobs` arguments.
Using MinGW, arch x86_64
Building for platform "windows", architecture "x86_64", target "editor".
AttributeError: 'SConsEnvironment' object has no attribute 'RES':
File "/home/akien/Godot/godot/SConstruct", line 1072:
SConscript("platform/" + env["platform"] + "/SCsub") # Build selected platform.
File "/home/akien/.local/lib/python3.13/site-packages/SCons/Script/SConscript.py", line 661:
return method(*args, **kw)
File "/home/akien/.local/lib/python3.13/site-packages/SCons/Script/SConscript.py", line 598:
return _SConscript(self.fs, *files, **subst_kw)
File "/home/akien/.local/lib/python3.13/site-packages/SCons/Script/SConscript.py", line 287:
exec(compile(scriptdata, scriptname, 'exec'), call_stack[-1].globals)
File "/home/akien/Godot/godot/platform/windows/SCsub", line 54:
res_obj = env.RES(res_target, res_file)
That's with mingw-w64 on Linux, building for Windows.
There was a problem hiding this comment.
Gaaaaaaah
Change of plans, I'm shrinking this PR to add the missing checks like LINKFLAGS explicitly. Everything else can be handled in another PR
There was a problem hiding this comment.
Gaaaaaaah
Change of plans, I'm shrinking this PR to add the missing checks like
LINKFLAGSexplicitly. Everything else can be handled in another PR
I'd prefer to revert the PR that created the regression instead. We shouldn't add hacks on top of hacks.
There's potentially a lot of configuration we're losing in the process by re-setting the mingw tool, and we shouldn't try to play whack-a-mole with it.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Wait wait, don't revert it. That PR already fixed other, existing hacks elsewhere, like needing to append .exe to filesystems in #98968. I'll cook up a proper solution here after all
EDIT: Ahh, I see the PR that's gonna "solve" this; that might do the trick
|
Superseded by #101726 |
MinGWsupport & checks #98105This PR removes a later
mingwtool call by having the call handled in SConstruct instead. It turns out thatmingwas a tool was already declared in that file, but wasn't always being processed thanks to these platform arguments being parsed before validation. While this PR fixes that by moving the tools a bit further down, it's still hacky by necessity; we will eventually need a much broader refactor of our SCons structure.