Skip to content

[WIP] Improve SCons AddOption option handling #3436

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

Closed
wants to merge 2 commits into from

Conversation

mwichmann
Copy link
Collaborator

@mwichmann mwichmann commented Aug 30, 2019

Currently, an option-with-arg added to a project with AddOption has some problems. If the form --opt=arg is used, handling is correct, but if the whitespace=separated form --opt arg is used, arg ends up considered as one of the targets supplied on the command line. If the option is specified with nargs > 1, there will always be whitespace as separators, and so these always fall into the hole, and depending on the rest of the command line, probably will trigger an error that the option required nargs arguments.

Work in progress: test cases added, solution still needed.

Closes #2748
Closes #2805
Closes #2977

Signed-off-by: Mats Wichmann [email protected]

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation

@mwichmann mwichmann changed the title Improve SCons AddOption option handling [WIP] Improve SCons AddOption option handling Aug 30, 2019
@mwichmann
Copy link
Collaborator Author

I've rejiggered reparse_local_options in SConsOptions.py to change the flow and pick the following arguments out of the "leftover arguments" list. That fixes the part of the problem where the option gets separated from the arguments and so not processed properly, but it leaves the problem where the option-args still go on the targets list. That happens in Main, and I don't see how to avoid it: the logic there is if it doesn't start with a dash, and contains an = sign, it goes on the value arguments list, if it doesn't it goes on the targets list.

@mwichmann
Copy link
Collaborator Author

mwichmann commented Oct 7, 2019

I actually think that disallowing space separators and nargs >1 is the cleanest fix. (for the latter case, --opt=foo,bar,baz and parse the single "word" of option-argument yourself would work. there are already scons options that do that... see --tree)

@mwichmann
Copy link
Collaborator Author

Some more poking today and I think this is very difficult problem to solve because of the chicken-and-egg situation involved. The command line is processed once, before the SConscripts are read, because you have to. (a) you might have an argument that tells you a different file to use than SConstruct, and (b) you have to gather up the targets and command-line Variables settings, because both of those things need to be available for checking/acting on in SConscripts. Currently the SCons-overridden _process_long_opt just ignores an option it doesn't recognize, instead of erroring; that defers the processing until later. When you're reading the SConscripts, and you see an AddOption, you call a function which tries to see if that option was on the command line, and if so collects it. But because at this point the two other classes of unused arg words had to already be made available for the SConscript context, it's already too late: if you are going to add --foo, and --foo something was on the command line, by the time you know to process that, something has already been presented as part of COMMAND_LINE_TARGETS and BUILD_TARGETS. The only thing that actually works is if the option-argument was attached to the option word by the = sign.

This doesn't actually seem to have broken hundreds of users as one might expect if there were lots of users of the functionality, but there are several filed issues on the topic - three noted in the original message here. If we want to make it consistent I'm more convinced than before that the only clean way to do that is what is in the previous comment. I've confirmed the no-more-than-one-option-argument case needs only a two-line patch in SConsOptionParser.add_local_option and the other one is probably a two-line patch in reparse_local_options in the same class. Of course there would be a doc impact as well.

I can't see how this could break many people: the more-than-one option-argument case can't work (the tests added here show that); the one-option-argument-with-space case can work, although it's basically by accident - in some configurations it will fail but in some it works - and we still have that option-argument made available as a target as well, which is wrong. But is it conceivable there are users who depend on the one-arg case who don't look at the *TARGET variables and so are proceeding. So some thought is needed.

Currently, an option-with-arg added with AddOption has some problems.
If the form "--opt=arg" is used, handling is correct, but if the
form "--opt arg" is used, the arg ends up considered as one of
the targets supplied on the command line.  If the option is specified
with nargs > 1, there will always be spaces as separators, and
so these always fall into the hole.

This commit adds two test cases.

Closes SCons#2748
Closes SCons#2805
Closes SCons#2977

Signed-off-by: Mats Wichmann <[email protected]>
Collect nargs arguments and get them off the lefover-args list

Signed-off-by: Mats Wichmann <[email protected]>
@mwichmann
Copy link
Collaborator Author

This one is force-updated to keep from going completely stale. I'm going to close the PR presently, and keep the tracking branch.

@mwichmann mwichmann added the args_and_options options processing, arguments, get/setoption and their relationshiop label Mar 22, 2021
@mwichmann
Copy link
Collaborator Author

I'm going to close this one. I'll resubmit the testcases so we have them around.

@mwichmann mwichmann closed this Jun 29, 2021
mwichmann added a commit to mwichmann/scons that referenced this pull request Jun 29, 2021
The two tests originally proposed in SCons#3436, now withdrawn,
are added along with a new .exclude_tests to not run them,
since they'd currently be in a failing state.

Needed to fix runtest.py, wasn't completely correctly
processing .exclude_tests files.

Signed-off-by: Mats Wichmann <[email protected]>
mwichmann added a commit to mwichmann/scons that referenced this pull request Jun 29, 2021
The two tests originally proposed in SCons#3436, now withdrawn,
are added along with a new .exclude_tests to not run them,
since they'd currently be in a failing state.

Needed to fix runtest.py, wasn't completely correctly
processing .exclude_tests files.

Signed-off-by: Mats Wichmann <[email protected]>
mwichmann added a commit to mwichmann/scons that referenced this pull request Jun 29, 2021
The two tests originally proposed in SCons#3436, now withdrawn,
are added along with a new .exclude_tests to not run them,
since they'd currently be in a failing state.

Needed to fix runtest.py, wasn't completely correctly
processing .exclude_tests files.

Signed-off-by: Mats Wichmann <[email protected]>
@mwichmann mwichmann mentioned this pull request Jun 29, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
args_and_options options processing, arguments, get/setoption and their relationshiop
Projects
None yet
1 participant