SCons: Add only selected platform's opts to env #44433
Conversation
Otherwise we can get situations where platform-specific opts with the same name can override each other depending on the order at which platforms are parsed, as was the case with `use_static_cpp` in Linux/Windows. Fixes godotengine#44304. This also has the added benefit that the `scons --help` output will now only include the options which are relevant for the selected (or detected) platform.
akien-mga
added
bug
topic:buildsystem
cherrypick:3.x
| @@ -131,14 +128,13 @@ opts.Add(BoolVariable("werror", "Treat compiler warnings as errors", False)) | |||
| opts.Add(BoolVariable("dev", "If yes, alias for verbose=yes warnings=extra werror=yes", False)) | |||
| opts.Add("extra_suffix", "Custom extra suffix added to the base filename of all generated binary files", "") | |||
| opts.Add(BoolVariable("vsproj", "Generate a Visual Studio solution", False)) | |||
| opts.Add(EnumVariable("macports_clang", "Build using Clang from MacPorts", "no", ("no", "5.0", "devel"))) | |||
There was a problem hiding this comment.
I moved this to platform/osx/detect.py. I think it should still work but I haven't tested.
The same can't be done easily for vsproj which is used in SConstruct and assumed to be in env (could be changed but that's a more significant refactoring).
| # Add platform-specific options. | ||
| for opt in detect.get_opts(): | ||
| opts.Add(opt) | ||
| opts.Update(env_base) |
There was a problem hiding this comment.
We call opts.Update() three times in total when a platform is selected, and twice otherwise. I'm not sure if this has any significant impact on build time (doesn't seem to have much for scons --help on Linux at least).
On the other hand we generate the Help() only once as I remember this as being significant (CC @Xrayez to test and confirm maybe, I remember that all this is slower on Windows).
There was a problem hiding this comment.
Updating environment and generating help text have little performance impact, it's more about parsing Python scripts, which just happens to be slow on Windows...
|
Seems to break build on macOS completely, with this change |
|
Yep, I'll push a fix in a minute. Things were pretty brittle due to the |
Fixes a pre-existing bug that godotengine#44433 exposed. It's pretty hacky, but we use `platform` in `env` both as an optional command line option (instead it can be autodetected, or passed via the `p` alias, and on Linux it might be overridden if you pass one of the convenience alias values), and as the reference value for what platform we're building on. Thus we override `env_base["platform"]` with the autodetected or validated platform, but any call to `opts.Update(env_base)` overrides it with the original command line option... causing e.g. godotengine#44448. The proper fix would be to refactor all this so that we don't reuse `env["platform"]` for platform detection (it could instead be e.g. `env.platform` as a member variable which holds the validated value), but for now I'm tapering over the immediate breakage. Fixes godotengine#44448 and other breakages induced by godotengine#44433.
|
Cherry-picked for 3.2.4. |
akien-mga
removed
the
cherrypick:3.x
Fixes a pre-existing bug that godotengine#44433 exposed. It's pretty hacky, but we use `platform` in `env` both as an optional command line option (instead it can be autodetected, or passed via the `p` alias, and on Linux it might be overridden if you pass one of the convenience alias values), and as the reference value for what platform we're building on. Thus we override `env_base["platform"]` with the autodetected or validated platform, but any call to `opts.Update(env_base)` overrides it with the original command line option... causing e.g. godotengine#44448. The proper fix would be to refactor all this so that we don't reuse `env["platform"]` for platform detection (it could instead be e.g. `env.platform` as a member variable which holds the validated value), but for now I'm tapering over the immediate breakage. Fixes godotengine#44448 and other breakages induced by godotengine#44433. (cherry picked from commit 8f66039)
Otherwise we can get situations where platform-specific opts with the same name
can override each other depending on the order at which platforms are parsed,
as was the case with
use_static_cppin Linux/Windows.Fixes #44304.
This also has the added benefit that the
scons --helpoutput will now onlyinclude the options which are relevant for the selected (or detected) platform.