godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix platform selection, custom tools overrides, and user overrides of `use_mingw`

Open shana opened this issue 1 year ago • 5 comments

TL;DR

This PR lets 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. The android and web toolsets that were previously hardcoded in SConstruct are moved to the respective get_flags() method in detect.py. Users can also override the tools via custom_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, target and custom_tools options 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_flags different 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

scons
scons: Reading SConscript files ...
Automatically detected platform: windows
Initializing SCons environment for "windows" with the following tools: "default".
Auto-detected 16 CPU cores available for build parallelism. Using 15 cores by default. You can override it with the -j argument.
Found MSVC version 14.3, arch x86_64
Building for platform "windows", architecture "x86_64", target "editor".

Windows host, non-mingw bash shell, use_mingw=yes

🚀  scons use_mingw=yes
scons: Reading SConscript files ...
Automatically detected platform: windows
Initializing SCons environment for "windows" with the following tools: "mingw".
Auto-detected 16 CPU cores available for build parallelism. Using 15 cores by default. You can override it with the -j argument.

            Running from base MSYS2 console/environment, use target specific environment instead (e.g., mingw32, mingw64, clang32, clang64).

Windows host, mingw64 bash shell, use_mingw=yes

$ scons use_mingw=yes
scons: Reading SConscript files ...
Automatically detected platform: windows
Initializing SCons environment for "windows" with the following tools: "mingw".
Auto-detected 16 CPU cores available for build parallelism. Using 15 cores by default. You can override it with the -j argument.
Using MinGW, arch x86_64
Building for platform "windows", architecture "x86_64", target "editor". 

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.

scons use_mingw=yes p=android
scons: Reading SConscript files ...
Initializing SCons environment for "android" with the following tools: "clang clang++ as ar link".
Auto-detected 16 CPU cores available for build parallelism. Using 15 cores by default. You can override it with the -j argument.
Checking for Android NDK...
Building for platform "android", architecture "arm64", target "template_debug".

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_mingw needs to be passed in as well, so scripts that check it directly will do the right thing.

scons platform=windows target=template_debug use_mingw=yes custom_tools=mingw,msvs
scons: Reading SConscript files ...
Initializing SCons environment for "windows" with the following tools: "mingw msvs".
Auto-detected 16 CPU cores available for build parallelism. Using 15 cores by default. You can override it with the -j argument.
Using MinGW, arch x86_64
Building for platform "windows", architecture "x86_64", target "template_debug".

Mac host

scons: Reading SConscript files ...
Automatically detected platform: macos
Initializing SCons environment for "macos" with the following tools: "default".
Auto-detected 10 CPU cores available for build parallelism. Using 9 cores by default. You can override it with the -j argument.
Building for macOS 11.0+.
MoltenVK found at: /Users/shana/VulkanSDK/1.3.261.1/MoltenVK/MoltenVK.xcframework/macos-arm64_x86_64/
Building for platform "macos", architecture "arm64", target "editor".

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.

scons p=ios
scons: Reading SConscript files ...
Initializing SCons environment for "ios" with the following tools: "default".
Auto-detected 10 CPU cores available for build parallelism. Using 9 cores by default. You can override it with the -j argument.
Building for platform "ios", architecture "arm64", target "template_debug".

Early build environment initialization issues

use_mingw

The use_mingw flag is a very special case, in that it both forces replacing the default toolset with the mingw toolset 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, because use_mingw is 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_mingw to be set at all times. The problem is that use_mingw is 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 set use_mingw by 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_mingw directly 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_mingw forcebly replaces the custom tools used to initialize the environment, and not all tools enabled by the default scons toolset are enabled in the mingw toolset. 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, the msvs tool, which enables Scons to generate visual studio projects for the current build configuration, is not enabled when using the mingw toolset. In order to allow a vs project to be generated when the mingw toolset 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 the use_mingw flag clobbers the Scons environment toolset becomes a bit of a problem.

target flag

The target flag 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 of template_debug, will have the editor builders loaded for non-editor builds that don't specify a target in the command line.

The env.editor_build flag is being set before reading platform flags. This means that platforms that set a different default target via get_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

shana avatar Sep 15 '23 16:09 shana

~I just noticed that the Windows build GitHub Action is failing with AttributeError: 'SConsEnvironment' object has no attribute 'MSVSProject':, which is a symptom of the Environment not having the right tool set up. I can't reproduce this at all - on my git bash shell, with the exact same arguments, it builds and generates the VS project just fine. Can anyone else repro this failure?~

~The job is https://github.com/shana/godot/actions/runs/6200292856/job/16834775240, and it's building with scons platform=windows target=editor tests=true verbose=yes warnings=extra werror=yes module_text_server_fb_enabled=yes debug_symbols=no vsproj=yes windows_subsystem=console~

~Update: Nevermind, found the issue, fixing...~

Update 2: Ok, it's fixed, we're good now.

shana avatar Sep 15 '23 16:09 shana

Can you write a brief how to test plan?

fire avatar Oct 03 '23 20:10 fire

Compile as you would usually, make sure it works.

akien-mga avatar Oct 03 '23 20:10 akien-mga

I found an issue compiling with msys, so I'm fixing that, will update soon.

shana avatar Oct 05 '23 15:10 shana

Ok, let's try this again @fire @akien-mga I've tested this on windows and mac with mingw and non-mingw builds for windows, android, mac and ios, along with other non-public platforms that introduce some edge cases to these build combos. I found more bugs with the current build initialization order that I've fixed. I've added more information to the PR body, including what to test with example outputs.

shana avatar Oct 12 '23 13:10 shana