Split out CPPDEFINES handling in Append methods

[mwichmann]

Rather than having lots of special-case code for CPPDEFINES in four separate routines, add a new _add_cppdefines function to handle it, paying attention to append/prepend, unique/duplicating, and keep-original/replace-original. The existing special case handing was then removed from Append and AppendUnique (it was never present in Prepend and PrependUnique anyway - see #3876, but these now get it due to a call to the new function).

Tuple handling is now consistent with list handling: a single tuple is treated as macro names to add, not as a name=value pair. A tuple or list has to be a member of a containing tuple or list to get the macro=value treatment. This may affect some existing usage. macro=value tuples without a value can now be entered either in (macro,) form or (macro, None) form.

Internally, whenever append/prepend is done, existing contents are forced to a deque, which allows efficient adding at either end without resorting to the tricks the Prepend functions currently do (they still do these tricks, but only in non-CPPDEFINES cases). As a result, values from a dict are not stored as a dict, which has some effect on ordering: values will be consistently ordered, but the ones from the dict are no longer necessarily sorted.

In SCons/Defaults.py, processDefines no longer sorts a dict it is passed, since Python now preserves dict order. This does not affect the E2E test for CPPDEFINES - since those all call an Append routine, CPPDEFINES will always be a deque, and so processDefines never sees a dict in that case. It could well affect real-life usage - if the setup of CPPDEFINES was such that it used to contain a dict with multiple entries, the order might change (sorting would have presented the keys from that dict in alphabetical order). This would lead to a one-time rebuild for actions that change (after that it will remain consistent).

In the E2E test CPPDEFINES/append.py some bits were reformatted, and the handler routine now accounts for the type being a deque - since the test does a text comparison of internally produced output, it failed if the word "deque" appeared. Some new test cases were also added to exercise strings with spaces embedded in them.

Changes were made to the expected output of the E2E test. These reflect changes in the way data is now stored in CPPDEFINES, and in some cases in order. Most of these do not change the meaning (i.e. "result" changes, but "final" output is the same). These are the exceptions:

• "appending a dict to a list-of-2lists", AppendUnique case: order now preserved as entered (previously took the order of the appended dict)

• "appending a string to a dict", Append case: not stored as a dict, so ordering is as originally entered.

• "appending a dict to a dict", Append case: no longer merge into a dict, so this is now an actual append rather than a merge of dicts which caused the uniquing effect even without calling AppendUnique (arguably the old way was incorrect).

A new test/CPPDEFINES/prepend.py is added to test Prepend* cases.

append.py and prepend.py are structured to fetch the SConstruct from a fixture file.

append.py got an added test in the main text matrix, a string of the macro=value form. The same 5x5 maxtrix is used in the new prepend.py test as well ("expected" values for these had to be added as well).

Cosmetically, append and prepend now print their test summary so strings have quotation marks - the "orig" lines in the expected output was adjusted. This change looks like:

-   orig = FOO, append = FOO
+   orig = 'FOO', append = 'FOO'

The other tests in test/CPPDEFINES got copyright updating and reformatting, but otherwise do not change.

Documentation updated to clarify behavior.

Fixes #4254
Fixes #3876

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

[bdbaddog]

Since this can change the CPPDEFINES, we should add a section at the top with
NOTE: The following usages of CPPDEFINES have changed
env.Append(CPPDEFINES,('a','b') ) now yields -Da -Db instead of -Da=b
etc..
Should be a fairly small set of these to clearly spell out what's changed?

[bdbaddog]

This should probably be multiple bullets.

[mwichmann]

Can do that, but it's really convoluted - see the long report in the one issue of the various inconsistencies I had discovered. So there would be a bunch of "now get (b) instead of (a), but only if (c); if (d) the behavior was already (b); if (e), this previously did not work at all".

[bdbaddog]

I think that would be valuable. This whole PR is really about fixing some corner cases (and reorganizing the code so it's more sane), so explaining which corner cases are now fixed (changed) would be useful for anyone who updated and saw their build change, and/or has a build not doing what they expect and sees that the newer version solves that.

Bigger organizations often need a forcing function to move to a newer version.

[bdbaddog]

value instead of definition?

[mwichmann]

I thought I was writing it in terms of how the C standard expresses those... although now I look, the standard uses "replacement" rather than "definition".

[bdbaddog]

4.5.0

[bdbaddog]

adding deque here will most likely just confuse users. Many of whom aren't strong python programmers. I'd leave it just the list.

[bdbaddog]

Can this no longer be done with a list?

[mwichmann]

It can, it's just awkward to keep repeating multiples all over the place. It would be nice to use "sequence" as that covers all of tuple, list, UserList, etc. but it also includes str, which is not a data type that can be used for a name, value pair.

[mwichmann]

I pushed a rewording of this part.

[bdbaddog]

deque?

[mwichmann]

The code takes the initial value given to CPPDEFINES as-is, but once you further Append/Prepend it takes the liberty of converting to a type useful to it. It always did that, but the chosen compound type depends on the sequence of events - could be a list, a dict, etc. Or in the case of AppendUnique, it converted it to a type incorrectly in some cases. Now we consistently get a deque, allowing the processing to be sane. Yes, as noted elsewhere, it could be a list instead but that shouldn't really be important to the user. Which type is really only exposed if you dig, as it happens here trying to show an example.

[bdbaddog]

4.5.0

[bdbaddog]

deque?

[bdbaddog]

Curious: Why use deque instead of a list to hold the defines?

[mwichmann]

Curious: Why use deque instead of a list to hold the defines?

This is a natural fit for a deque since it's designed to be added to from either end with Prepend and Append. Prepending a deque is as fast as appending, and this way don't have to play silly games (reverse the list, append to it, reverse back... that kind of thing).

[bdbaddog]

Would you expect all the examples in this sample code https://github.com/bdbaddog/scons-bugswork/tree/main/4254
(Derived from description in Issue #4254 )
To work with your current code here? I'm seeing some issues.

This is the example run on current master:

% python ~/devel/scons/git/as_scons/scripts/scons.py -n
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
gcc -o main_case0.o -c -Dz=case2 main.c
gcc -o main_case1.o -c "-D('z', 'case1')" main.c
gcc -o main_case2.o -c -Dz=case2 -Da -Db main.c
gcc -o main_case4.o -c -Dcase4 -Dz=case4 main.c
gcc -o main_case5.o -c -Dz -Dcase5 main.c
scons: done building targets.

This is the example run against this branch (Note FIXED=1 enables case 3 which crashes in master)

% python ~/devel/scons/git/mwichmann/scripts/scons.py -n FIXED=1
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
gcc -o main_case0.o -c -Dz=case2 main.c
gcc -o main_case1.o -c "-D('z', 'case1')" main.c
gcc -o main_case2.o -c -Dz -Dcase2 -Da -Db main.c
gcc -o main_case3.o -c -Dz -Dcase3 -Dsimple_string main.c
gcc -o main_case4.o -c -Dcase4 main.c
gcc -o main_case5.o -c -Dz -Dcase5 main.c
scons: done building targets.

[mwichmann]

I've pushed a rebase of this (had to force), dealing with Util having been refactored meanwhile. A couple of small changes, plus test expansion to make sure those cases are covered, coming next. Then back to the RELEASE/CHANGES material.

[mwichmann]

FWIW, this is the output from the current version:

scons: Building targets ...
gcc -o main_case0.o -c -Dz=case2 main.c
gcc -o main_case1.o -c -Dz -Dcase1 main.c
gcc -o main_case2.o -c -Dz -Dcase2 -Da -Db main.c
gcc -o main_case3.o -c -Dz -Dcase3 -Dsimple_string main.c
gcc -o main_case4.o -c -Dcase4 -Dz -Dcase4 main.c
gcc -o main_case5.o -c -Dz -Dcase5 main.c
scons: done building targets.

[bdbaddog]

FWIW, this is the output from the current version:

scons: Building targets ...
gcc -o main_case0.o -c -Dz=case2 main.c
gcc -o main_case1.o -c -Dz -Dcase1 main.c
gcc -o main_case2.o -c -Dz -Dcase2 -Da -Db main.c
gcc -o main_case3.o -c -Dz -Dcase3 -Dsimple_string main.c
gcc -o main_case4.o -c -Dcase4 -Dz -Dcase4 main.c
gcc -o main_case5.o -c -Dz -Dcase5 main.c
scons: done building targets.

So only case 0 is correct?

[mwichmann]

Define correct here. If you specifically wanted the output result -Dz=case2 the three ways to enter it are:

[('z','case2')]
{'z': 'case2'}
"z=case2"

Only your case0 uses one of those three forms.

Otherwise, I'm not sure what you're after... the appending is all being handled as I expect now.

[bdbaddog]

Updated logic to match current behavior when you append a tuple to yield -Dkey=value

[bdbaddog]

The only question mark is, do we change specifying CPPDEFINES initially as a tuple to yield -Dkey=value, or leave as is.
It would seem more consistent if it was changed.

Thoughts?

[mwichmann]

That note, basically, was already in place. Okay if you think it's helpful to have it at the top as well.

[bdbaddog]

We always put the "it may rebuild" warnings at the top. to try to minimize the TL;DR issue...

[mwichmann]

If you're okay with tuple specifically having a different meaning than list - personally, I think that is inconsistent. If so, then some further rework is needed (see below). What do you propose happens if someone tried to Append, say, ('FOO', ('BAR', '22'))? Currently that would end up expanding to "FOO=('BAR', '22')", whereas a list version of the same becomes ['FOO', 'BAR=22'].

Rework:

• docs need to describe that adding a tuple by itself becomes a valued-macro
• the test cases will need to be gone through to reset the expectations for the changed behavior.
• "initial value" needs to be handled. With the latest change, Appending a tuple when CPPDEFINES doesn't exist yet gives a different result than when CPPDEFINES has any value. Obviously, that's a no-go - user code isn't supposed to have to know if it already had a value. This happens in processDefines.
• currently, processDefines takes any sequence embedded in a sequence as a valued-macro. What should it do if it sees a non-tuple here? This is the pre-PR versionn (mine only combines the two conditions into a single is_Sequence check):

           elif SCons.Util.is_List(d) or isinstance(d, tuple):
               if len(d) >= 2:
                   l.append(str(d[0]) + '=' + str(d[1]))
               else:
                   l.append(str(d[0]))

[mwichmann]

Attaching the result of running the CPPDEFINES tests as they exist in the PR against a version that changes both the add-single-tuple and the processDefines-single-tuple behavior (the last CI run had only one of these two changes, so the results would not have been the same there)

Changed_tuple.txt

[mwichmann]

Even with the two changes there's still a hole: appending a tuple works as you want, and initializing to a single tuple works (because then processDefines just sees that tuple`), but initializing with a tuple and then appending (anything) to it does not (because at the time of the first append the original tuple is converted into the internal datatype first, losing its tuple status). You want me to keep going down this road?

[mwichmann]

One more question is what to do with strings - when to split on space and when not. We know a string entry inside a list is promised not to be split, so you can enter entries with spaces (that's not CPPDEFINES specific, of course). But:

initial 'FOO BAR=22' without adds -> ['FOO BAR=22']  # initial is not split if never added to
iniitial 'FOO BAR=22' Append ('B', 'DEBUG') -> ['FOO', 'BAR=22', 'B=DEBUG']  # initial has been split
initial None Append 'BAZ B=DEBUG' -> ['BAZ', 'B=DEBUG']  # addition has been split
initial 'FOO BAR=22' Append 'BAZ B=DEBUG' -> ['FOO', 'BAR=22', 'BAZ B=DEBUG']  # initial split, addition has not been split

Isn't this fun?

[bdbaddog]

Even with the two changes there's still a hole: appending a tuple works as you want, and initializing to a single tuple works (because then processDefines just sees that tuple`), but initializing with a tuple and then appending (anything) to it does not (because at the time of the first append the original tuple is converted into the internal datatype first, losing its tuple status). You want me to keep going down this road?

Seems like a tuple should always defined -Dkey=value and not change when somethings appended to it right?

[bdbaddog]

oopsie.. accidently closed.

[mwichmann]

Seems like a tuple should always defined -Dkey=value and not change when somethings appended to it right?

If the decision is that's what a tuple is, then it should be consistently so - see my quote from earlier:

Obviously, that's a no-go

[mwichmann]

For completeness, tweaked tests slightly so tests can run against master without the changes applied. Such a test run is attached here for comparison (these tests run "clean" on the current PR version):

LOG.txt

[mwichmann]

oops, spotted that I'd made an error in preparing the Prepend test (copy-paste error, hadn't changed one instance of Append to Prepend), and fixed it... and then didn't remember to go fix the "expected" text. Fix for that forthcoming.