scons incorrectly parses targets when command line options are used for variant_dir

[bdbaddog]

This issue was originally created at: 2014-09-24 20:02:33.
This issue was reported by: mfd.

mfd said at 2014-09-24 20:02:33

With the following structure:

scons_bug
|-- SConscript
|-- SConstruct
|-- t0
|   |-- SConscript
|   `-- t0.cpp
`-- t1
    |-- SConscript
    `-- t1.cpp

where SConstruct is:

AddOption(
    '--out',
    nargs=1,
    dest='out',
    action='store',
    help='directory to build into',
    default='out'
)
SConscript('SConscript', variant_dir=GetOption('out'), duplicate=0)

and SConscript is:

env = Environment()
Export('env')
SConscript('t0/SConscript')
SConscript('t1/SConscript')
Default(['t0', 't1'])

and t0/SConscript is:

Import('env')
t0_env = env.Clone()
t0_env.Program('t0', ['t0.cpp'])

and t1/SConscript is:

Import('env')
t1_env = env.Clone()
t1_env.Program('t1', ['t1.cpp'])

If I run scons t0 (or scons t1) I get the expected output (only the specified target is build and the output is built in "out").

scons t0 output:

scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
scons: building associated VariantDir targets: out/t0
g++ -o out/t0/t0.o -c t0/t0.cpp
g++ -o out/t0/t0 out/t0/t0.o
scons: `t0' is up to date.
scons: done building targets.

However, if I run scons --out foo t0 scons correctly picks up foo as the specified output directory (e.g. accessible via GetOption and output is indeed directed toward the foo directory). The problem is that it also appears to pick up foo as a target and hence builds the entire world before then building the specified target (which has actually already been built because the entire world was built so it just prints that it is up to date). In the real project "the world" is much much larger than t0 and t1 of course.

scons --out foo t0 output:

scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
scons: building associated VariantDir targets: foo
g++ -o foo/t0/t0.o -c t0/t0.cpp
g++ -o foo/t0/t0 foo/t0/t0.o
g++ -o foo/t1/t1.o -c t1/t1.cpp
g++ -o foo/t1/t1 foo/t1/t1.o
scons: building associated VariantDir targets: foo/t0
scons: `t0' is up to date.
scons: done building targets.

Notice that it builds foo as a target. If you change it to scons t0 --out foo the same thing happens except it builds foo/t0 first and then builds foo.

Interestingly enough though if you run scons --out=foo t0 (notice the = in the command) the behavior is correct (only t0 is built and it is built into the directory foo).

For the command scons --out=foo t0, optparse correctly returns {'out': 'foo'} as the options and ['t0'] as the args (it also does it correctly for all the other various ordering/permutations). That seems to imply that scons looks for the targets from the command line itself, outside of optparse (but I haven't looked at the code).

mfd said at 2014-09-24 20:05:26

Created an attachment (id=950)
example project showing problem

dirkbaechle said at 2014-09-25 08:19:05

Things seem to go wrong in ll. 959 of engine/SCons/Script/Main.py:

    # That should cover (most of) the options.  Next, set up the variables
    # that hold command-line arguments, so the SConscript files that we
    # read and execute have access to them.
    targets = []
    xmit_args = []
    for a in parser.largs:
        if a[:1] == '-':
            continue
        if '=' in a:
            xmit_args.append(a)
        else:
            targets.append(a)

Unfortunately, the fix isn't a simple one-liner...if done properly. Boolean arguments are a problem...so one would have to compare the option "type" (string or bool) for each found argument, and then decide whether a following argument is required that has to get skipped while compiling the list of targets. I'm also not sure how to handle options that have a callback method defined...so quite some work ahead there.

mfd attached scons_bug.tar.gz at 2014-09-24 20:05:26.

example project showing problem

[mwichmann]

This is also the reason options with nargs > 1 are not working. scons is mangling the arguments in some way before we get here, so it's likely most of the fix is not here. If you just call optparse to handle a option that takes one argument, it comes out the same however it's called. That is, for

parser.add_option("-f", "--file", dest="filename",
                  help="write output to FILE", metavar="FILE")

All of these ways of calling it come back with the same stuff in the first value returned by parse_args:

$ python3 test.py -f
{'filename': 'foo'}
$ python3 test.py --file foo
{'filename': 'foo'}
$ python3 test.py --file=foo
{'filename': 'foo'}

But try to do the same in SCons, and we see:

$ scons --file=foo
> /home/mats/github/scons/src/engine/SCons/Script/Main.py(983)_main()
-> for a in parser.largs:
(Pdb) p parser.largs
['--prefix=foo']
(Pdb) 

$ scons --file foo
> /home/mats/github/scons/src/engine/SCons/Script/Main.py(983)_main()
-> for a in parser.largs:
(Pdb) p parser.largs
['--prefix', 'foo']
(Pdb) 

That is, in the = separator case, we get one string in largs, in the blank separator case we get two. The logic noted above from Main.py doesn't seem to be right for either: in the one-word case we skip if it starts with a dash, so the contains-= never happens, and in the two-word case the following arg doesn't have an equal so it is added to targets instead of to args.

[mwichmann]

Okay, looks to me like the problem is actually here - SConsOptions.py, SConsOptionParser._process_long_opt: this method overrides the one from base OptionParser by adding this:

        try:
            opt = self._match_long_opt(opt)
        except optparse.BadOptionError:
            if self.preserve_unknown_options:
                # SCons-specific:  if requested, add unknown options to
                # the "leftover arguments" list for later processing.
                self.largs.append(arg)
                if had_explicit_value:
                    # The unknown option will be re-processed later,
                    # so undo the insertion of the explicit value.
                    rargs.pop(0)
                return
            raise

The method signature: def _process_long_opt(self, rargs, values)::
if called with rargs = ['--prefix=foo'] the whole thing is parked on self.largs and can correctly be processed later. If called with rargs = ['--prefix', 'foo'] this is where they get separated - --prefix goes into self.largs, while its corresponding argument foo stays on the mutable rargs, which the optparse base parser class is busy iterating over, and why scons will end up seeing them as just plain target arguments. At this stage, scons doesn't yet know about the effects of an AddOption - that's why it tries to bin those unrecognized args for later, when it starts to learn about them after reading the sconscripts. And because it doesn't know, it cannot correctly decide how many arguments to save for the later processing.

As written this cannot possibly work for long-opt option arguments that are space separated from the option. Maybe the solution is to document the behavior? That is, for added long options, only the --opt=arg form works, and nargs > 1 is disallowed.