Append/Prepend special CPPDEFINES handling problems

[mwichmann]

Multiple past issues (see for example #2300) has caused env.Append and env.AppendUnique to grow special-case code for handling CPPDEFINES. One simple example of the special-casing is that normally Appending a string value to a string-valued consvar causes the strings to be concatenated, but for CPPDEFINES, they must remain two separate strings, so the variable is converted to a list and the new string added to the list.

This is a collective issue to document several problems.

• [ ] the special-casing was only added for Append and AppendUnique, but not for the parallel Prepend and PrependUnique, meaning one set can have differing result types from the other.
• [ ] there are no unit tests for the special CPPDEFINES handling. They are tested in e2e tests so this may not be a huge problem, but it seems an inconsistency, at least.
• [ ] the special-casing in AppendUnique does not handle in the same way the case where both existing and added are list types. There's at least one case where this leads to different results, as in the example snippets below:

env.Append(CPPDEFINES = [{'FAT32':1}])
env.Append(CPPDEFINES = [{'ZLIB':1}])

del env['CPPDEFINES']
env.AppendUnique(CPPDEFINES = [{'FAT32':1}])
env.AppendUnique(CPPDEFINES = [{'ZLIB':1}])

which if run with prints after each append shows:

env['CPPDEFINES']=[{'FAT32': 1}]
env['CPPDEFINES']=[{'FAT32': 1}, {'ZLIB': 1}]

env['CPPDEFINES']=[{'FAT32': 1}]
env['CPPDEFINES']=[({'FAT32': 1},), ({'ZLIB': 1},)]

that is, the AppendUnique case has tuple-ified the CPPDEFINES elements while the Append case has not.

• [ ] Quoting is handled differently (this is not specific to CPPDEFINES):

env1.Append(CCFLAGS = '/this /is /a /test')
env2.AppendUnique(CCFLAGS = '/this /is /a /test')

giving in one case three args and the other a single quoted arg:

cl /Fotestfile.obj /c testfile.cpp /TP /nologo /this /is /a /test
cl /Fotestfile.obj /c testfile.cpp /TP /nologo "/this /is /a /test"

[mwichmann]

Assigning to myself to keep track of this one.

[greenbender]

This can lead to compile issues. The following SConscript generates incorrect CPPDEFINES due to the AppendUnique "tuple-ification" issue mentioned above.

$ cat SConscript
env = Environment()
env.Append(CPPDEFINES=['A'])
env.Append(CPPDEFINES={'B': None})
env.AppendUnique(CPPDEFINES=['C'])
test_c = env.Textfile('test.c', 'int main(void) { return 42; }')
env.Program('test', test_c)

Results in:

$ scons
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
gcc -o test.o -c -DA "-D{'B': None}" -DC test.c
<command-line>: error: macro names must be identifiers
scons: *** [test.o] Error 1
scons: building terminated because of errors.

[mwichmann]

Guess I need to get back to this. Somehow there seems to be excess complexity, and we're currently carrying the four independent implementations of adding to a construction var, which is just poor engineering. I wonder if it would make sense, since they do type conversion anyway, to uses a deque so prepending doesn't have to play games "for performance reasons" since inserting at the front of a plain list is rather expensive.

[mwichmann]

For the WIP branch I have, the example added here by @greenbender now results in this output:

$ scons -n
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
Creating 'test.c'
gcc -o test.o -c -DA -DB -DC test.c
gcc -o test test.o
scons: done building targets.

which looks much better.

[mwichmann]

And, for completeness, the example from the first checklist item example:

env['CPPDEFINES']=[{'FAT32': 1}]  ->  _CPPDEFFLAGS=-DFAT32=1
env['CPPDEFINES']=deque([{'FAT32': 1}, {'ZLIB': 1}])  ->  _CPPDEFFLAGS=-DFAT32=1 -DZLIB=1

env['CPPDEFINES']=[{'FAT32': 1}]  ->  _CPPDEFFLAGS=-DFAT32=1
env['CPPDEFINES']=deque([{'FAT32': 1}, {'ZLIB': 1}])  ->  _CPPDEFFLAGS=-DFAT32=1 -DZLIB=1

so... now equal, rather than different.

[mwichmann]

The fourth checklist item from the original issue has been moved to separate issue #4262 as it was not specific to CPPDEFINES.