Added warning message if using bad command line options, fixed mono options #53030
Conversation
|
I'm not sure if we should print a warning or abort with an error in this situation. Adding an error would break automated build scripts that happen to be passing unknown parameters, especially if the parameters are platform-specific (some people may still be passing them on all platforms). I guess we could do a middle ground: abort with an error on PS: I'd change the commit message to "Abort build if invalid SCons options are passed", so that we don't scare off people who are using |
|
This also uncovered an issue with the Mono variables, it was creating its own variable instance, so it was not part of the help text. Latest commit fixes that. |
I respect whatever you want to do, but my personal opinion is that will just lead to confusion when things go wrong, and give allowance for processes to continue doing things incorrectly. Is it that hard for other systems to pass the right vars? Wouldn't they want to know if they are passing the wrong thing? Warnings are basically ineffective in automated systems, no one looks unless its red. Sure if you want to not break previous branches, the backport could be less strict. |
|
mono has a compilation error now that the options are getting added to the main variables, I'll look into what went wrong and switch the hard fail to a warning with the message @Calinou suggested. |
|
I'm of two minds on error vs warning, both have pros and cons. I do agree that if users pass an invalid option, an error is the best way to tell them about it so they can fix their script. At the same time, it can be fairly easy to pass invalid options for a specific platform/build, because some options are platform or config-specific. For example someone might want to favor LLVM over GCC and pass Another potential option is that the removal of some options will lead to breakage of user scripts, e.g. if users were disabling the Maybe the best would be a way to show the warnings both at the start and at the end of the build? Users are likely to miss a warning at the start if they issue the build command and then go sword-fighting while it compiles, but if the warnings are summarized at the end of build, they'll see them. |
What about some colors instead to make it more noticeable? I noticed godot already colorizes some output so I can leverage existing color code. |
dmoody256
force-pushed
the
check_command_vars
branch
from
81c1a79 to
cfe69cf
Compare
|
Okay I dug into the mono compilation failure, and when mono options go to Update the env, they wipe out the current 'platform' var. I saw in several place you reset this var after its been wiped out. Really you should use a different var for getting the command line value and recording the platform in the environment. This makes it so you can update the env after new options are discovered, and not wipe out the existing 'platform'. I did find and replace "platform" -> "selected_platform" |
dmoody256
force-pushed
the
check_command_vars
branch
from
cfe69cf to
6309329
Compare
| @@ -748,6 +756,7 @@ if selected_platform in platform_list: | |||
| if conf.CheckCHeader(header[0]): | |||
| env.AppendUnique(CPPDEFINES=[header[1]]) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
does black not fix whitespace? I have been lazy about my whitespace expecting black to format it.
dmoody256
force-pushed
the
check_command_vars
branch
from
6309329 to
c4a22bd
Compare
dmoody256
changed the title
…options, refactored platform var to selected_platform
dmoody256
force-pushed
the
check_command_vars
branch
from
c4a22bd to
7ab4607
Compare
| @@ -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)) | |||
There was a problem hiding this comment.
That seems wrong to me, doesn't it change our platform command line option to selected_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.
yeah, this was a mass find + replace mistake. The build interface should remain the same, I'll fix it.
| env_base["platform"] = selected_platform # Must always be re-set after calling opts.Update(). | ||
| Help(opts.GenerateHelpText(env_base)) |
There was a problem hiding this comment.
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.
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?
|
Partially superseded by #55203. |
This add a check if the user accidentally used an invalid option on the command line.
There was a complaint and request in the godot devel chat about it.