Fix platform selection, custom tools overrides, and user overrides of use_mingw
#81700
Conversation
|
Update 2: Ok, it's fixed, we're good now. |
shana
force-pushed
the
shana/fix-mingw-platform-tools
branch
from
bb3b790
to
5472bdb
Compare
|
Can you write a brief how to test plan? |
|
Compile as you would usually, make sure it works. |
|
I found an issue compiling with msys, so I'm fixing that, will update soon. |
shana
force-pushed
the
shana/fix-mingw-platform-tools
branch
from
5472bdb
to
ad05813
Compare
shana
force-pushed
the
shana/fix-mingw-platform-tools
branch
5 times, most recently
from
668c28b
to
331d1a9
Compare
|
Ok, let's try this again @fire @akien-mga |
…bey overrides of use_mingw Let the platforms optionally override the default scons environment tools via a `custom_tools` flag, so the default SConstruct only needs to set the defaults and not worry about each individual platform requirements. Users can also override the tools via `custom_tools`. Add a temporary SCons environment to only process `use_mingw`, `platform`, `target` and `custom_tools` options from all the different places where they can be set, and then use those values to initialize the actual environment that we use to build, as well as determine which builder scripts to load based on the target. This fixes a related issue where, if a platform sets a default target in `get_flags` different from editor, and the user doesn't pass in the target in the command line, the `editor` target would be briefly selected during a portion of SConstruct execution, long enough to set the env.editor_build to true, causing editor files and defines to be set for non-editor builds. Fixes godotengine#60719
shana
force-pushed
the
shana/fix-mingw-platform-tools
branch
from
331d1a9
to
042d297
Compare
akien-mga
changed the title
use_mingw
There was a problem hiding this comment.
Small issue. Incidentally, this is the same issue as is present in options like dev_mode. For instance, if in custom.py, dev_mode=yes verbose=no is specified, the final value of verbose will be yes, even though the explicit setting is supposed to take priority. I therefore suggest fixing both issues at the same time.
SConstruct
Outdated
| platform_dict = dict(platform_flags[selected_platform]) | ||
| for option in opts.options: | ||
| # Use the platform flag value if it's not set in the environment already (i.e. the user hasn't overridden it). | ||
| if env_base[option.key] == option.default and option.key in platform_dict: |
There was a problem hiding this comment.
If a user explicitly sets an option to the default value (via command line or custom.py, and the platform flags set it to a different value, the platform flags will "take priority" in this case, because this check will think the user did not set the value.
Some different approach is needed, in which the platform flags are calculated first, and passed as the default values when building the options, for every option.
There was a problem hiding this comment.
Changing the default values for the options depending on the selected platform, is a design decision with slightly wider implications than what this PR is trying to accomplish, which is to fix bugs in the early option parsing.
Changing the default values means explaining to the user why the defaults are suddenly different; updating the documentation to explain why they can change; and it can potentially break existing user workflows that expect the default values to be the same regardless of the platform.
I'm not saying it's not a potentially useful change, mind you! There is some sense to deciding that the platform is the one defining the defaults, and therefore the defaults should be set by the platform, and not the other way around. So, I wouldn't mind opening a PR with a potential change to do that, and having a discussion about it, yeah.
In the meantime, for this particular issue you raised, we can handle that without changing the defaults, at least for these very early options that control the early build environment. The problem here is that we can't actually tell whether the user has set a value for an option - whatever that value is - because Scons doesn't differentiate between "value == default because the user hasn't set any value" and "value == default because the user set it to that".
There are two potential ways to fix that, for this particular part of the initialization:
-
We can use our own Variables implementation that DOES track the difference between an option that has a default implied value and an option with an explicit user value, or
-
We don't set any defaults on the options at this stage of the initialization, so if they're empty, that means the user hasn't set a value, and we can safely use the value that the platform sets. If neither user nor platform set anything, we can then use the "real" default value that we already use in the full options set later in the code. This won't affect our help text later on, because that comes from the full set of options with actual default values.
I'm going to do the second one, because that's easy and will solve our immediate issue. And it will actually put everything in place for us to talk about changing the default values on a per-platform basis in the future - it would be very easy to inject the values that are returned from the platform flags here into the full option set that we define later, if we want to do that (later, in another PR).
There was a problem hiding this comment.
I don't know what you mean by "default options per platform", and I wasn't talking about that anyway. I was talking about: If the platform sets a flag, and the user does too, the user's selection should take priority.
If you succeed in making this work using None as the default (and later override it with the real default), that's great. But, I think it might not work for options that go through conversion, e.g. boolean options, because it would try to convert None to boolean and fail. But there's only one way to find out.
If you do get it to work, note that the fix should also be applied to any use of ARGUMENTS and get_cmdline_bool below. i.e. they all need to be replaced with a check of "If the value is None, replace it with this value".
P.S. I did suggest re-implementing Variables in RocketChat, but Akien was opposed.
There was a problem hiding this comment.
You said
Some different approach is needed, in which the platform flags are calculated first, and passed as the default values when building the options, for every option.
This implies that the default values for the Options change depending on the selected platform. That's what I was referring to when I said it was a larger design decision than just respecting the user flags over the platform flags.
I'm not changing any usages of get_cmdline_bool or arguments right now because I'm not touching any flags that would be overridden by those direct reads, not right now anyways. This is already a complicated change due to how many things use_mingw and target affect, I don't want to add additional unrelated changes on top of it.
Once this one is good to go, we can do more fixes for other flags that aren't beyond properly obeyed. Not all of them are being handled in the same way, so they all require specific fixes.
I've pushed some fixes to properly obey the user flags over the platform flags.
There was a problem hiding this comment.
The platform flags already override the default flags. There's no change there, larger or otherwise. The difference is that they currently do it for non-default flags as well.
As it is, you already copied the code that handles platform_flags from the bottom to the top, changed the top version to check the default value (and now, to check vs empty string), while leaving the bottom version as-is. That means the initial check for platform settings respects custom.py, but the actual usage does not. Meaning that, in the worst case, the settings used for building can be different than the ones used to load the tools.
Also, the empty string solution feels like a step in the wrong direction. None would have been nice if it worked, but empty string feels too fragile. It's replacing a bad hack with a worse hack.
After thinking about it for a while, there is a clean solution that doesn't require rewriting Variable. Since Python doesn't have access specifiers. As such, it's possible to retroactively change the default value of a variable by changing opts.options[i].default where opts.options[i].key is the option's name. The new default can then be applied by calling opts.Update. Notably, this can be done any number of times for the same environment. This way, several overrides for the default values can be applied, without overriding user values.
While it may seem that a complete fix to the problem is out of scope for this PR, a partial fix that is so hacky it would need to be reverted later, is better off not being merged. Even if it does need to be done in two PRs, the PR fixing the default values issue has to come first, and this one has to be based on top of it. Otherwise, a large part of this PR will simply have to be reverted later on.
Make sure that if the user sets a value equal to the default and the platform sets a value different from the default, that the user value is respected.
There was a problem hiding this comment.
Tried the PR with my usual build command on Linux, it errors out:
pyston-scons -j7 p=linuxbsd dev_build=yes dev_mode=yes linker=mold scu_build=all 2>&1 | tee -a build.log
scons: Reading SConscript files ...
KeyError: 'use_mingw':
File "/home/akien/Projects/godot/godot.git/SConstruct", line 139:
if env_base[option]:
File "/home/akien/Projects/godot/toolchains/pyston_2.3.5/lib/python3.8-pyston2.3/site-packages/SCons/Environment.py", line 588:
return self._dict[key]
| env_base.disabled_modules = [] | ||
| env_base.module_version_string = "" | ||
| env_base.msvc = False | ||
| # We'll parse these as strings first, so we can tell if they've been set explicitely. |
There was a problem hiding this comment.
Typo which makes the CI checks fail.
| # We'll parse these as strings first, so we can tell if they've been set explicitely. | |
| # We'll parse these as strings first, so we can tell if they've been set explicitly. |
|
I need to spend more time testing this PR (after rebasing it), but please note some recent changes I've made which might partially supersede some of the changes here (but not all):
This should remove the need for some of the more workaround-y changes, at least from a quick assessment. |
TL;DR
This PR lets the platforms optionally override the default scons environment tools via a
custom_toolsflag, so the default SConstruct only needs to set the defaults and not worry about each individual platform requirements. The android and web toolsets that were previously hardcoded in SConstruct are moved to the respectiveget_flags()method in detect.py. Users can also override the tools viacustom_tools.In order to properly initialize the SCons environment with the right toolset, this adds an initial temporary SCons environment that only processes
use_mingw,platform,targetandcustom_toolsoptions from all the different places where they can be set. These values are parsed and then used to initialize the actual environment that we use to build, as well as determine which builder scripts to load based on the target.This also fixes multiple related issues where, if a platform sets a default target in
get_flagsdifferent from editor, and the user doesn't pass in the target in the command line, the build will be partially configured to be an editor build, setting wrong defines and source files (see "target flag" section below)Fixes #60719
How to test this
The important bits to test are builds with a combination of use_mingw, custom tools, and default target set in the platform flags. Here is expected output for some of these configurations:
Windows host
Windows host, non-mingw bash shell, use_mingw=yes
Windows host, mingw64 bash shell, use_mingw=yes
Windows host, Android platform, use_mingw=yes
Android sets its own custom toolset for SCons, so use_mingw flag can be set, but the SCons toolset is controlled by the platform.
Android also sets a different default target, so the build output should not include any
editor/files.Advanced scenario: Windows host, Windows platform, mingw64 bash shell, use_mingw=yes, custom toolset
Enabling the "msvs" SCons tool for a mingw build requires passing the full toolset list to
custom_tools, so the SCons environment is correctly initialized.use_mingwneeds to be passed in as well, so scripts that check it directly will do the right thing.Mac host
Mac host, iOS platform, default target set by platform
Running this build into a log file and then grepping the log file to make sure there are no editor sources included.
Early build environment initialization issues
use_mingw
The
use_mingwflag is a very special case, in that it both forces replacing thedefaulttoolset with themingwtoolset for Scons environment initialization, and is also used to determine the format of compiler flags (msvc style or gcc/clang style). This is usually not a big issue, becauseuse_mingwis used to switch between windows+msvc and windows+mingw, so having both the toolset and the compiler check done from one option is ok.Certain platforms (namely consoles) use a mix of windows+non-msvc compilers, so they require
use_mingwto be set at all times. The problem is thatuse_mingwis initially only read from the command line, at the point where it's used to determine the SCons toolset to pass to the environment initialization. This means that platforms cannot setuse_mingwby default in their platform flags, as these flags are ignored at this point and only read later, once the environment is already initialized with the wrong toolset.Issue #60719 is a symptom of the way that we're currently reading
use_mingwdirectly from the command line and ignoring any user overrides until it's too late - the flag gets set in env eventually, but by then the environment has already been created with the wrong toolset.Another problem is that
use_mingwforcebly replaces the custom tools used to initialize the environment, and not all tools enabled by thedefaultscons toolset are enabled in themingwtoolset. Scons only initializes build tools like compilers and linkers in the mingw toolset, while the default toolset initializes a lot more tools, depending on the detected host platform. For example, themsvstool, which enables Scons to generate visual studio projects for the current build configuration, is not enabled when using themingwtoolset. In order to allow a vs project to be generated when themingwtoolset is selected, the environment has to be initialized with["mingw","msvs"]. This is, again, probably not a big issue for most mingw users, but since console platforms are often a mix of windows+clang+visual studio support, the way that theuse_mingwflag clobbers the Scons environment toolset becomes a bit of a problem.target flag
The
targetflag is being used to load editor builders (needed when building for the editor), but this particular check only looks at the flag in the command line, not the flag in the platform flags or in the environment. This means that platforms like ios, that sets a default target oftemplate_debug, will have the editor builders loaded for non-editor builds that don't specify a target in the command line.The
env.editor_buildflag is being set before reading platform flags. This means that platforms that set a different default target viaget_flags()will get editor sources and defines set for non-editor builds that don't specify a target in the command line.Contributed by W4Games