-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SCons: Fix CCFLAGS
, LINKFLAGS
, etc. command line overrides
#86964
SCons: Fix CCFLAGS
, LINKFLAGS
, etc. command line overrides
#86964
Conversation
@@ -507,21 +510,11 @@ if selected_platform in platform_list: | |||
env.extra_suffix += "." + env["extra_suffix"] | |||
|
|||
# Environment flags | |||
CCFLAGS = env.get("CCFLAGS", "") | |||
env["CCFLAGS"] = "" | |||
env.Append(CCFLAGS=str(CCFLAGS).split()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand why this was being done in the first place. Reading the original value, wiping it, and then appending it as an array, why? The change below is not quite equivalent, as it appends without wiping the value first and doesn't split the string when appending...
might there be a case where one of CCFLAGS/CFLAGS/CXXFLAGS/LINKFLAGS doesn't actually exist or isn't treated as an array, so unless we wipe it and explicitely append as an array, the append below will fail/not do the right thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered too, and tried env.Append(CCFLAGS=env.get("CCFLAGS", ""))
but I found that it was appending the string twice (-Wall -Wextra
would turn to -Wall -Wextra-Wall -Wextra
).
Because the trick is that env.Append(CCFLAGS=...)
and env["CCFLAGS"]
are the same variable. So rename helped solve this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is/was scons ever reading these from the OS environment variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. SCons makes things confusing by calling its own state "Environment" but the only values we read from the OS environment are those:
env_base.PrependENVPath("PATH", os.getenv("PATH"))
env_base.PrependENVPath("PKG_CONFIG_PATH", os.getenv("PKG_CONFIG_PATH"))
if "TERM" in os.environ: # Used for colored output.
env_base["ENV"]["TERM"] = os.environ["TERM"]
$ export CXX=clang++; CCFLAGS="-Wbogus" /usr/bin/scons verbose=yes
[Initial build] g++ -o platform/linuxbsd/godot_linuxbsd.linuxbsd.editor.x86_64.o -c -std=gnu++17 -fno-exceptions -pipe -O2 -Wall -Wshadow -Wno-misleading-indentation -Wno-return-type -isystem thirdparty/glad -DTOOLS_ENABLED -DDEBUG_ENABLED -DNDEBUG -DNO_EDITOR_SPLASH -DSOWRAP_ENABLED -DTOUCH_ENABLED -DFONTCONFIG_ENABLED -DALSA_ENABLED -DALSAMIDI_ENABLED -DPULSEAUDIO_ENABLED -D_REENTRANT -DDBUS_ENABLED -DSPEECHD_ENABLED -DXKB_ENABLED -DJOYDEV_ENABLED -DUDEV_ENABLED -DLINUXBSD_ENABLED -DUNIX_ENABLED -D_FILE_OFFSET_BITS=64 -DX11_ENABLED -DVULKAN_ENABLED -DRD_ENABLED -DGLES3_ENABLED -DMINIZIP_ENABLED -DBROTLI_ENABLED -DCLIPPER2_ENABLED -DZSTD_STATIC_LINKING_ONLY -DUSE_VOLK -DVK_USE_PLATFORM_XLIB_KHR -DGLAD_ENABLED -DEGL_ENABLED -DGLAD_GLX_NO_X11 -DHAVE_MNTENT -Ithirdparty/freetype/include -Ithirdparty/libpng -Ithirdparty/volk -Ithirdparty/vulkan -Ithirdparty/vulkan/include -Ithirdparty/zstd -Ithirdparty/zlib -Ithirdparty/clipper2/include -Ithirdparty/brotli/include -Ithirdparty/linuxbsd_headers -Iplatform/linuxbsd -I. platform/linuxbsd/godot_linuxbsd.cpp
I think this makes sense! It certainly makes it much clearer about what is and isn't an internal scons variable vs our custom build options. |
Matching the changes in godotengine/godot#86964.
SConstruct
Outdated
LINKFLAGS = env.get("LINKFLAGS", "") | ||
env["LINKFLAGS"] = "" | ||
env.Append(LINKFLAGS=str(LINKFLAGS).split()) | ||
env.Append(CPPDEFINES=env.get("cppdefines", "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to have to be explicit about this being an array by calling split()
, I'm running into issues where a quoted list of defines is being interpreted as one single define in some environments (mostly in GitHub actions, see https://github.com/W4Shared/godot-nir-builds/actions/runs/7474876144/job/20341918395#step:6:34)
env.Append(CPPDEFINES=env.get("cppdefines", "")) | |
env.Append(CPPDEFINES=env.get("cppdefines", "").split()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I was afraid of that.
I'm worried that env.get(...).split()
might sometimes fails because env.get()
doesn't necessarily return a string. Notably in recent SCons version, I've seen it sometimes we a deque
... and thus even the previous str(...).split()
wouldn't work because stringification of deque
seems to do something like deque(...)
.
My Python-fu isn't great there, but we'd need to find something that we're confident will work for both strings and deques I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah well I guess it was a deque
when we were getting env["CPPDEFINES"]
. For a command line argument that's not conflicting with a SCons Environment collection, I guess we might be good assuming that we're dealing with a string?
diff --git a/SConstruct b/SConstruct
index e51719b3f8..777faa3fd7 100644
--- a/SConstruct
+++ b/SConstruct
@@ -510,6 +510,8 @@ if selected_platform in platform_list:
env.extra_suffix += "." + env["extra_suffix"]
# Environment flags
+ print(type(env.get("cppdefines")))
+ print(type(env["CPPDEFINES"]))
env.Append(CPPDEFINES=env.get("cppdefines", ""))
env.Append(CCFLAGS=env.get("ccflags", ""))
env.Append(CXXFLAGS=env.get("cxxflags", ""))
$ scons cppdefines="test test1"
<class 'str'>
<class 'collections.deque'>
6595708
to
f220a43
Compare
Also adds `CPPDEFINES` which allows passing new pre-processor defines, letting SCons handle passing `-D` or `/D` based on the compiler.
f220a43
to
0d18947
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Also adds
CPPDEFINES
which allows passing new pre-processor defines, letting SCons handle passing-D
or/D
based on the compiler.Following this discussion: https://github.com/godotengine/godot-nir-static/pull/4/files#r1443246323
I had to rename the variables like
CCFLAGS
/LINKFLAGS
toccflags
/linkflags
because command line options share the same namespace with the actual SCons env variables, and replacing those on the fly was messy (required using a temporary variable) and would probably also destroy some important initial setup done by SCons (or here with the addition ofcppdefines
, done by us before these options are parsed).This breaks compatibility (
scons CCFLAGS="-Wall"
will no longer work, needs to bescons ccflags="-Wall"
), but it makes it clearer that those options are Godot-specific and just append to their uppercase SCons env counterpart.The options support space-separated arguments, e.g.
scons cppdefines="MY_DEFINE MY_OTHER_DEFINE"
. Appending such a string (or deque, depending on SCons version) toenv["CPPDEFINES"]
automatically handles splitting arguments.CC
/CXX
/LINK
stay uppercase as those actually overrideenv["CC"]
, etc. It's still bug prone as some platformdetect.py
sometimes resetenv["CC"]
manually, but improving that is for a future effort, if the need arises.CC @shana