AddOption(shortOption ...) does not fully add the short option

[DeeeeLAN]

Describe the bug
If you try and add a short option (-x), it doesn't process far enough to actually be usable from the command line. It does get processed enough to show up in the help message, however, which can lead to visible confusion by the end user.

Required information

• Link to SCons Users thread discussing your issue.
  • discussed in discord
• Version of SCons
  • 4.0.1
• Version of Python
  • 3.8.1
• Which python distribution if applicable (python.org, cygwin, anaconda, macports, brew,etc)
  • python.org
• How you installed SCons
  • built by source I believe
• What Platform are you on? (Linux/Windows and which version)
  • Linux, CentOS 7
• How to reproduce your issue? Please include a small self contained reproducer. Likely a SConstruct should do for most issues.

AddOption('-x', '--longX',
          action='store_true', 
          dest='xvar', 
          default=False,
          help="Test short option"
print(GetOption('xvar'))

• How you invoke scons (The command line you're using "scons --flags some_arguments")

scons -h
scons -x

Note, my preference is to fix this rather than disallow it in the docs because if the variable I want to add is a file, the long option prevents me from using tab completion. If I try to do --longX=path/to/file.x I have to type it manually, but if I can do -x path/to/file.x, I can tab complete to enter the file path. That is how I initially stumbled upon this issue.

[mwichmann]

you have dest='xvar' and then GetOption('var'), I'm presuming that's just a typo?

I poked a little bit after the discord discussion, and the scons specialization of optparse's OptionParser seems to be entirely oriented to thinking these are long options - I don't even see where the short option is going. Will take some investigation.

[DeeeeLAN]

Yep, sorry, just a typo.

That is what I think I was seeing while poking through the source as well. It's not too complicated of code though, so I might play around with it a bit too.

[mwichmann]

Note, my preference is to fix this rather than disallow it in the docs because if the variable I want to add is a file, the long option prevents me from using tab completion. If I try to do --longX=path/to/file.x I have to type it manually, but if I can do -x path/to/file.x, I can tab complete to enter the file path. That is how I initially stumbled upon this issue.

This is going to be problematic anyway. Because on the first scan of the arglist you haven't read the SConscripts, and thus don't know about -x path, the path part is likely to get separated out and passed on in COMMAND_LINE_TARGETS. By the time you hit the AddOption that tells SCons -x takes a matching arg, that's already happened...

[DeeeeLAN]

Ah, yes. I suppose the built in short args are able to get around this since they are known at startup. Would it make sense for SCons on the second path if a short arg is detected to go and pull the next N items (depending on the option) out of the COMMAND_LINE_TARGETS list? Assuming those items haven't been processed yet. I don't know the order of operations, but I was under the impression the options get processed before anything else. So the file path would be placed in COMMAND_LINE_TARGETS temporarily, but it would be gone before it matters.

[mwichmann]

Yeah. I have some patches that attempt to improve on the behavior a bit. One set is stashed in #3436, but that's a rather, umm mature PR at this point and I don't quite remember what I did. In general, you might notice I personally think this piece of the codebase is a bit hokey.

[DeeeeLAN]

I created a pull request in #3799. It ended up being a pretty simple solution to get the short options working, we just needed to override the _process_short_opts function and through the mismatches back on the stack just like we do with the _process_long_opts. Whether this might have any unintended side effects, I am not sure. I will say that I tried using it in my flow by specifying a -T /path/to/test type short option and it worked fine. The test path wasn't pulled in as a separate argument.

[DeeeeLAN]

Regarding the chicken and egg situation. The option that stands out in my mind to allow white space would be to:

1. process the command line for know options and their associated values as currently occurs
2. do a first pass through all the scons files and process any additional AddOption() calls, but defer all other processing of the file until this step is complete
3. do a second pass through the command line and pull out the remaining options and values
4. process the remainder of the command line as targets or variables
5. continue on processing the construct file.

I don't know if this approach would have a significant performance impact. I think enforcing an order of operations would clean things up a bit though. options -> variables -> targets. This could be documented and enforced as such, although I can't imagine many cases where that order would need to be disturbed.

Another option would be to go and yank the option values back out of the variables and targets lists once they are known, but that could possibly cause some unintended consequences if the script was able to act on that information before it was removed. I don't know if that is possible or not though. This would be much simpler to implement I believe.

[DeeeeLAN]

I took a stab at my proposed option 2 above, which can be seen in 72b8e02. I think it should be safe from the chicken/egg situation because at the point in time the values are yanked out of the lists, the user code has not started processing yet, as far as I can tell while working through this. I added _Remove_Argument and _Remove_Target functions which allow the option parser function to remove values from the respective lists, if they are present. It will only remove one if it is present multiple times, so if you have a situation like scons -x TARG TARG for some reason, it will work fine.

I also ended up removing the reparse_local_options function completely. As far as I could tell, all that function was doing was screwing up the order of the command line arguments, which was what was breaking the "two AddOption" situation.

I have expanded the two tests you added in #3436 significantly and they both passing with these changes. I am curious to know if you can think of any other potential edge cases.

At this point, I think this section of the codebase might be slightly less hokey. 😁

[mwichmann]

I liked where this was going in the first round of changes in #3799, and would guess that had a good chance of being accepted by the maintainer. This later chunk does make me somewhat nervous - sconscripts are allowed to root around in *TARGETS/ARGUMENTS/ARGLIST and I'm a little nervous at fiddling them on the fly - could someone have made a decision based on something that later gets removed from one of those by a subsequent AddOption?

[DeeeeLAN]

The first round of changes got us to short form parity with where we are now with long options. We would be able to use them, but only in the attached form -xFoo, because spaces still wouldn't work. We could start but just merging those changes in though, since they definitely wouldn't break anything.

There is definitely a concern about how reading the SConscript files is handled. Main.py lines 1003-1004 are where the leftover args are set as targets or variables. line 1082 is when the argument are re-parsed. The SConscript files are read between then, but not processed yet. Can the variable/target list have an impact on anything while just reading the files? I thought all decision making occurs after, while actually processing the files.

I think in whatever form this is handled, the additional targets would need to be removed, because if the value is an actual target, we wouldn't want to run it.

I can add some more tests that try and replicate the situation to see if we run into issues. I am not sure what exactly to look for though. Do you have a possible use-case in mind that could be something to look for? I am thinking something like an SConscript makes a decision assuming a particular target or variable is on the command line, but the command line actually passes it as a value to an option, so it would get removed. I am just not sure what that decision process should be.

If you tried to stick AddOption calls behind a variable check for some reason, there could be issues for sure I think:

if ARGUMENTS.get('foo', 0):
    AddOption(--'foo', ...)

Or even more targets:

if 'foo' in BUILD_TARGETS:
    BUILD_TARGETS.extend(['a', 'b', 'c'])

I don't think we can fully be safe from this though. Because even if we did an "AddOption only" passthrough of the SConscript files, there could still be additional options behind arguments like above, so you can't assume everything that is left is only targets or variables.

[DeeeeLAN]

For what it's worth, those two test additions both pass, so I am thinking we might be safe. It looks like all additional processing of the command line is deferred until after the options are fully settled.

[DeeeeLAN]

Okay, I identified a test case in 642a7ba that still fails. So the options are actually clearing their args while reading the sconscript files, not on line 1082 when the reparse function call is made. What this means is that if the options are parsed before the other checks (if 'A' in BUILD_TARGETS:...), it will work how we expect. If, however, that code happens first, then it will be executed, which is the case that we are concerned about. The most recent commit adds a failing test that demonstrates this.

Due to this behavior, I think the only way to really make this work reliably would be two passes through the files at whatever impact that might have. The first pass would be used to update the options, targets, and arguments lists, and the second pass through would use the updated targets and arguments. This would require saving the initial set of variables and restoring everything that changed for the second pass through.

Alternatively, I think it would not be unreasonable to document the current behavior and leave it as is. It isn't really worse in any way than how it was before, since the old style still works fine. The only situation that would cause an issue is if an option argument is supplied that matches the name of a target or variable and that target or variable (the full A=B variable, not just A or B, which would be unusual) was used for control flow within the SConscript file and that code was placed before the AddOption code, and the user doesn't see the (to be written) note in the user guide or man page warning them about this situation.

For completeness sake I am going to explore executing the files twice and see if I can get an example running that allows the test to pass.