Skip to content
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.

Sign up for GitHub

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: Refactor profile option for custom.py-style overrides #91794

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+37 −22
Draft

SCons: Refactor profile option for custom.py-style overrides #91794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37cf266
SCons: Process platform-specific flags earlier
akien-mga May 10, 2024
2621189
SCons: Refactor `profile` option for `custom.py`-style overrides
akien-mga May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 35 additions & 20 deletions SConstruct
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,16 +178,7 @@ env.SConsignFile(File("#.sconsign{0}.dblite".format(pickle.HIGHEST_PROTOCOL)).ab

# Build options

customs = ["custom.py"]

profile = ARGUMENTS.get("profile", "")
if profile:
if os.path.isfile(profile):
customs.append(profile)
elif os.path.isfile(profile + ".py"):
customs.append(profile + ".py")

opts = Variables(customs, ARGUMENTS)
opts = Variables(args=ARGUMENTS)

# Target build options
if env.scons_version >= (4, 3):
Expand All @@ -209,6 +200,11 @@ opts.Add(BoolVariable("debug_paths_relative", "Make file paths in debug symbols
opts.Add(EnumVariable("lto", "Link-time optimization (production builds)", "none", ("none", "auto", "thin", "full")))
opts.Add(BoolVariable("production", "Set defaults to build Godot for use in production", False))
opts.Add(BoolVariable("threads", "Enable threading support", True))
opts.Add(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure but before was only one file? What is the reason to have several profile files? You intend to create something like modularity? One common file and several platform specific?

"profile",
"Path to one or several comma-separated Python files with variable declarations to override default arguments",
"custom.py",
)

# Components
opts.Add(BoolVariable("deprecated", "Enable compatibility code for deprecated and removed features", True))
Expand Down Expand Up @@ -242,7 +238,7 @@ opts.Add("vsproj_name", "Name of the Visual Studio solution", "godot")
opts.Add("import_env_vars", "A comma-separated list of environment variables to copy from the outer environment.", "")
opts.Add(BoolVariable("disable_3d", "Disable 3D nodes for a smaller executable", False))
opts.Add(BoolVariable("disable_advanced_gui", "Disable advanced GUI nodes and behaviors", False))
opts.Add("build_profile", "Path to a file containing a feature build profile", "")
opts.Add("build_profile", "Path to a file containing a JSON feature build profile generated by Godot", "")
opts.Add(BoolVariable("modules_enabled_by_default", "If no, disable all modules except ones explicitly enabled", True))
opts.Add(BoolVariable("no_editor_splash", "Don't use the custom splash screen for the editor", True))
opts.Add(
Expand Down Expand Up @@ -305,6 +301,25 @@ opts.Add("rcflags", "Custom flags for Windows resource compiler")
# in following code (especially platform and custom_modules).
opts.Update(env)

# Process user profiles with custom arguments.
custom_profiles = env["profile"].split(",")
profiles_to_load = []
custom_args = {}
for profile in custom_profiles:
if os.path.isfile(profile):
profiles_to_load.append(profile)
elif os.path.isfile(profile + ".py"):
profiles_to_load.append(profile + ".py")
if profiles_to_load:
for profile in profiles_to_load:
profile_args = {}
with open(profile, "r", encoding="utf-8") as f:
code = f.read()
exec(code, {}, profile_args)
custom_args = {**custom_args, **profile_args}
# Update the variables based on the profiles.
opts.Update(env, {**ARGUMENTS, **custom_args, **env})
Comment on lines +320 to +321
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need to reason a bit on the order of merging overlapping arguments from command line, custom.py, and then potential further additions to the env. Will need to do more testing on this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that cli args should override profile args. Cli args + profile args = user args. User args should override env... Your current code does exactly opposite (prefers env over profile args over command line args)

And I would suggest to have separate variable for storing both cli and profile args (user_args or something like it). It will be handy in several places. And IIUC whole if (key not in ARGUMENTS or ARGUMENTS[key] == "auto") and key not in custom_args: could be then written shorter with user_args


# Copy custom environment variables if set.
if env["import_env_vars"]:
for env_var in str(env["import_env_vars"]).split(","):
Expand Down Expand Up @@ -374,6 +389,15 @@ if env["platform"] in platform_opts:
for opt in platform_opts[env["platform"]]:
opts.Add(opt)

# Platform-specific flags.
# These can sometimes override default options, so they need to be processed
# as early as possible to ensure that we're using the correct values.
flag_list = platform_flags[env["platform"]]
for key, value in flag_list.items():
# Allow command line and profiles to override platform flags.
if (key not in ARGUMENTS or ARGUMENTS[key] == "auto") and key not in custom_args:
env[key] = value

# Update the environment to take platform-specific options into account.
opts.Update(env, {**ARGUMENTS, **env.Dictionary()})

Expand Down Expand Up @@ -568,17 +592,8 @@ if env["build_profile"] != "":
print_error('Failed to open feature build profile: "{}"'.format(env["build_profile"]))
Exit(255)

# Platform specific flags.
# These can sometimes override default options.
flag_list = platform_flags[env["platform"]]
for key, value in flag_list.items():
if key not in ARGUMENTS or ARGUMENTS[key] == "auto": # Allow command line to override platform flags
env[key] = value

# 'dev_mode' and 'production' are aliases to set default options if they haven't been
# set manually by the user.
# These need to be checked *after* platform specific flags so that different
# default values can be set (e.g. to keep LTO off for `production` on some platforms).
if env["dev_mode"]:
env["verbose"] = methods.get_cmdline_bool("verbose", True)
env["warnings"] = ARGUMENTS.get("warnings", "extra")
Expand Down
2 changes: 1 addition & 1 deletion platform/android/detect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def get_min_target_api():

def get_flags():
return {
"arch": "arm64", # Default for convenience.
"arch": "arm64",
"target": "template_debug",
"supported": ["mono"],
}
Expand Down
2 changes: 1 addition & 1 deletion platform/ios/detect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def get_doc_path():

def get_flags():
return {
"arch": "arm64", # Default for convenience.
"arch": "arm64",
"target": "template_debug",
"use_volk": False,
"supported": ["mono"],
Expand Down
Loading