[WIP] Add a check for bad opts which are abbrev of good

[mwichmann]

Adds a post-processing step called after sconscript processing is done which checks if there are any leftover options. Previously, some options would sneak through silently if they were an abbreviation of another option added by AddOption.

Marked as WIP to see if this is a suitable approach. There was also some discussion of having this transition in gradually as a warning/deprecationwarning.

Note: at the moment this has some surprises. Test test/option-unknown.py tries a long option where no AddOption calls have taken place at all, and it still triggers the abbreviation warning.

Fixes #3653

Signed-off-by: Mats Wichmann mats@linux.com

Contributor Checklist:

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

[dmoody256]

MongoDB slack channels are complaining about this. Any progress?

Proposed fix from someone in the mongodb slack channel, but I suppose its not this simple:

diff --git a/src/third_party/scons-3.1.2/scons-local-3.1.2/SCons/Script/SConsOptions.py b/src/third_party/scons-3.1.2/scons-local-3.1.2/SCons/Script/SConsOptions.py
index e7a3fc1800..f9138482a4 100644
--- a/src/third_party/scons-3.1.2/scons-local-3.1.2/SCons/Script/SConsOptions.py
+++ b/src/third_party/scons-3.1.2/scons-local-3.1.2/SCons/Script/SConsOptions.py
@@ -280,7 +280,8 @@ class SConsOptionParser(optparse.OptionParser):
             had_explicit_value = False
         try:
-            opt = self._match_long_opt(opt)
+            if opt != self._match_long_opt(opt):
+                raise optparse.BadOptionError("cannot use abbreviated option %s, use %s instead" % (opt, self._match_long_opt(opt)))
         except optparse.BadOptionError:
             if self.preserve_unknown_options:
                 # SCons-specific:  if requested, add unknown options to

Now you would get:

scons: done reading SConscript files.
usage: scons [OPTION] [TARGET] ...
SCons Error: no such option: cannot use abbreviated option --variables-file, use --variables-files

[mwichmann]

I kind of got lost in several overlapping changesets that have to do with the broken option handling. Will swing back and take a look - it might work out to be "that simple" just for this issue.

[acmorrow]

@mwichmann - If the simple fix is acceptable, I think it would be worthwhile getting a version of it committed so it can make the next 4.0 point release. That would let us easily cherry-pick it into our vendored SCons, and remove a very frustrating user experience related bug.

[mwichmann]

Okay, I'll have a dig.

[acmorrow]

@mwichmann - Did your digging reveal anything along the lines of the simple fix?

[mwichmann]

My shovel went missing temporarily....

[acmorrow]

@mwichmann and @dmoody256 - I think it would be worthwhile to revisit this and see if the proposed simple fix really does work. It keeps burning people when they leave terminal letters off of important options and then are deeply perplexed when their build breaks. If a more comprehensive change is required, that's fine. But if the little fix works, it really seems like a worthy UX improvement without much risk.

[mwichmann]

The patch detects at least the case in the testcase I put together, which means it would be useful. For anyone reading this - should this be an error or a warning? It seems I had coded this PR to issue a warning, but bad options are usually an error, so I'm now more inclined to go with what the patch collected from Mongo slack proposed.

[acmorrow]

I think as a stopgap before the behavior of intrinsic options and user declared options can be harmonized, it should be an error. Warnings have a tendency to scroll right by as the build starts and get missed. Since the failure mode here is that user declared options get silently ignored when they are prefix-y, I think there is a strong case for making it an error.

[bdbaddog]

Error seems correct.
Though you might check if there's an Alias() or a Node() with the name in question.

Though weird.. that's possible..
And fairly easy to check?

[mwichmann]

surely that wouldn't trigger on a --longopt?

[mwichmann]

Fascinating... I asked on Discord not too long ago why errors emit "scons: ***" instead of "scons: Error:". That's presumably some ancient decision that we can't find. But SCons.Script.SConsOptions actually uses a variant of the latter form - in SConsOptionParser.error, and it's been that way for some time.

[mwichmann]

My inclination is to push a new PR rather than try to adapt this one, since this has some other stuff in it and the commit history would end up confusing. There's still some reason why I wanted the post-process approach but right now I can't (quickly) prove it's necessary.

[mwichmann]

Withdrawing this in favor of #3784, will archive for reference.