-
-
Notifications
You must be signed in to change notification settings - Fork 310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Append/Prepend special CPPDEFINES handling problems #3876
Comments
Assigning to myself to keep track of this one. |
This can lead to compile issues. The following $ 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. |
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. |
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. |
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. |
The fourth checklist item from the original issue has been moved to separate issue #4262 as it was not specific to |
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 SCons#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 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 added to also 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 SCons#4254 Fixes SCons#3876 Signed-off-by: Mats Wichmann <mats@linux.com>
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 SCons#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 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 added to also 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 SCons#4254 Fixes SCons#3876 Signed-off-by: Mats Wichmann <mats@linux.com>
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 SCons#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 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 added to also 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 SCons#4254 Fixes SCons#3876 Signed-off-by: Mats Wichmann <mats@linux.com>
According to `timeit`, this is slightly faster. Also, `.copy()` will work when `V` is a `collections.deque`, but `[:]` fails in that case. SCons issue SCons/scons#3876 (first released in SCons 4.5.0) changed the storage of the `CPPDEFINES` variable from a `list` to a `deque`, which broke use of `env.get(name, [])[:]`, since `deque` cannot copy itself using `[:]`. Switch to using `.copy()` to obtain a shallow copy. ``` >>> # timing comparison >>> timeit.timeit(stmt='a.copy()', setup='a = [1, 2, 3, 4]') 0.05652548000216484 >>> timeit.timeit(stmt='a[:]', setup='a = [1, 2, 3, 4]') 0.06801111809909344 ``` ``` >>> # deque cannot copy using slice notation >>> a = [1, 2, 3] >>> from collections import deque >>> b = deque([1, 2, 3]) >>> a.copy() [1, 2, 3] >>> a[:] [1, 2, 3] >>> b.copy() deque([1, 2, 3]) >>> b[:] Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: sequence index must be integer, not 'slice' ``` Reported-by: Kreeblah <#704>
Multiple past issues (see for example #2300) has caused
env.Append
andenv.AppendUnique
to grow special-case code for handlingCPPDEFINES
. One simple example of the special-casing is that normallyAppend
ing a string value to a string-valued consvar causes the strings to be concatenated, but forCPPDEFINES
, 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.
Append
andAppendUnique
, but not for the parallelPrepend
andPrependUnique
, meaning one set can have differing result types from the other.CPPDEFINES
handling. They are tested in e2e tests so this may not be a huge problem, but it seems an inconsistency, at least. And the e2e tests only test Append functions.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:which if run with prints after each append shows:
that is, the
AppendUnique
case has tuple-ified theCPPDEFINES
elements while theAppend
case has not.The text was updated successfully, but these errors were encountered: