-
-
Notifications
You must be signed in to change notification settings - Fork 22.4k
Added warning message if using bad command line options, fixed mono options #53030
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,8 @@ import sys | |
import time | ||
from collections import OrderedDict | ||
|
||
import SCons | ||
|
||
# Local | ||
import methods | ||
import glsl_builders | ||
|
@@ -57,7 +59,7 @@ methods.save_active_platforms(active_platforms, active_platform_ids) | |
|
||
custom_tools = ["default"] | ||
|
||
platform_arg = ARGUMENTS.get("platform", ARGUMENTS.get("p", False)) | ||
platform_arg = ARGUMENTS.get("selected_platform", ARGUMENTS.get("p", False)) | ||
|
||
if os.name == "nt" and (platform_arg == "android" or methods.get_cmdline_bool("use_mingw", False)): | ||
custom_tools = ["mingw"] | ||
|
@@ -111,6 +113,7 @@ if profile: | |
customs.append(profile + ".py") | ||
|
||
opts = Variables(customs, ARGUMENTS) | ||
env_base["opts"] = opts | ||
|
||
# Target build options | ||
opts.Add("p", "Platform (alias for 'platform')", "") | ||
|
@@ -233,7 +236,7 @@ if selected_platform in ["linux", "bsd", "x11"]: | |
|
||
# Make sure to update this to the found, valid platform as it's used through the buildsystem as the reference. | ||
# It should always be re-set after calling `opts.Update()` otherwise it uses the original input value. | ||
env_base["platform"] = selected_platform | ||
env_base["selected_platform"] = selected_platform | ||
|
||
# Add platform-specific options. | ||
if selected_platform in platform_opts: | ||
|
@@ -242,7 +245,6 @@ if selected_platform in platform_opts: | |
|
||
# Update the environment to take platform-specific options into account. | ||
opts.Update(env_base) | ||
env_base["platform"] = selected_platform # Must always be re-set after calling opts.Update(). | ||
|
||
# Detect modules. | ||
modules_detected = OrderedDict() | ||
|
@@ -296,8 +298,6 @@ methods.write_modules(modules_detected) | |
|
||
# Update the environment again after all the module options are added. | ||
opts.Update(env_base) | ||
env_base["platform"] = selected_platform # Must always be re-set after calling opts.Update(). | ||
Help(opts.GenerateHelpText(env_base)) | ||
Comment on lines
-299
to
-300
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit of a pain that SCons puts user-provided arguments and dev-defined environment configuration in the same data structure. Ideally we should be able to use the most natural term "platform" both for the argument that users are passing, and for the internal variable we want to check to see what platform we're building for. It's a bit annoying that this forces us either to do hacks like I used to have, or use a wordy "selected_platform" everywhere (which breaks compatibility for user scripts and custom modules, so I'm not too fond of the prospect). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well scons is not forcing you, you are telling the Variables obj to update them in that environment. I suppose Update could have an optional interface of a list of Variables to ignore in a given update. It essentially what you doing with the set after update. The alternative method here is that you make multiple Variables instances and then each update to the environment is a separate one. You will need to join them together to generate the complete help text. Want me to rewrite this with that implementation? |
||
|
||
# add default include paths | ||
|
||
|
@@ -478,7 +478,7 @@ if selected_platform in platform_list: | |
) | ||
# Apple LLVM versions differ from upstream LLVM version \o/, compare | ||
# in https://en.wikipedia.org/wiki/Xcode#Toolchain_versions | ||
elif env["platform"] == "osx" or env["platform"] == "iphone": | ||
elif env["selected_platform"] == "osx" or env["selected_platform"] == "iphone": | ||
vanilla = methods.is_vanilla_clang(env) | ||
if vanilla and cc_version_major < 6: | ||
print( | ||
|
@@ -718,6 +718,15 @@ if selected_platform in platform_list: | |
env.vs_incs = [] | ||
env.vs_srcs = [] | ||
|
||
if opts.UnknownVariables(): | ||
|
||
methods.print_color(sys, "red", "ERROR: Unknown command line variables:") | ||
for name, val in opts.UnknownVariables().items(): | ||
methods.print_color(sys, "red", " '{}={}'".format(name, val)) | ||
methods.print_color(sys, "red", "Please check valid variables usage with '--help' option.") | ||
|
||
Help(opts.GenerateHelpText(env)) | ||
|
||
Export("env") | ||
|
||
# Build subdirs, the build order is dependent on link order. | ||
|
@@ -748,6 +757,7 @@ if selected_platform in platform_list: | |
if conf.CheckCHeader(header[0]): | ||
env.AppendUnique(CPPDEFINES=[header[1]]) | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does black not fix whitespace? I have been lazy about my whitespace expecting black to format it. |
||
elif selected_platform != "": | ||
if selected_platform == "list": | ||
print("The following platforms are available:\n") | ||
|
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.
That seems wrong to me, doesn't it change our
platform
command line option toselected_platform
? That would break compatibility and force us to update all documentation, as well as force users to update all their build scripts, I wouldn't do it unless it's strictly needed.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.
yeah, this was a mass find + replace mistake. The build interface should remain the same, I'll fix it.