-
-
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
CPPDEFINES handling of tuple arguments inconsistent #4254
Labels
Append
Issues in Append/Prepend + Unique variants
Comments
mwichmann
added a commit
to mwichmann/scons
that referenced
this issue
Nov 14, 2022
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>
3 tasks
mwichmann
added a commit
to mwichmann/scons
that referenced
this issue
Dec 16, 2022
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>
After lots of hashing out of things in the related PR (#4263), tuples will always be treated as a valued macro - by themselves or as members of a list. This is different from my proposal of "most consistent" being to treat a tuple like a list, and only give it a special meaning if it is a member of a list. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Filed so this slightly confusing situation is recorded somewhere.
Handling of
CPPDEFINES
gets some special-casing. InAppend
andAppendUnique
(but not, currently, in thePrepend
cousins - see #3876) they get some special handling, and then, along with normal substitution and further processing (*ixes), they get processed bySCons.Defaults.processDefines()
. TheprocessDefines
call is currently unique to the processing of_CPPDEFFLAGS
.One of the ways you can declare a C preprocessor macro of the form macro=value through
CPPDEFINES
is to use a tuple of length at least two; the first and second values are then combined byprocessDefines
into thatNAME=Value
form. Unfortunately, it is not clearly specified whether a single tuple argument should have this effect, or only a tuple that is an element of an iterable, and so it's kind of hard to determine what is expected behavior.CPPDEFINES
contains a single tuple (for example, by doing anAppend
to an uninitializedCPPDEFINES
, although direct assignment, or initializing in theEnvironment
call would have exactly the same effect),processDefines
falls through to its "default" case, which is just to stringify the contents (l = [str(defs)]
). Since we now have a single string, the eventual output of having a tuple like("FOO", 2)
is-D('FOO', 2)
, which is syntactically incorrect for both gcc-style compilers and msvc. We might expect either treating it as a macro=value tuple:-DFOO=2
, or as simply an iterable of values just as if it were a list:-DFOO -D2
.Append
a list value toCPPDEFINES
containing a single tuple, theAppend
routine does some try/except magic and ends up inserting the tuple into the front of the list. Thus, we could haveCPPDEFINES = [('FOO', 2), 'BAR', 'BAZ']
.processDefines
turns this into['FOO=2', 'BAR', 'BAZ']
, and the final expansion is-DFOO=2 -DBAR -DBAZ
.Append
a single string value toCPPDEFINES
containing a single tuple, it fails withAttributeError: 'str' object has no attribute 'insert'
which is probably a surprise - you asked to "append" and you get a failure on "insert".Append
a tuple to any existing value, the macro=value form is assumed. Say we started withCPPDEFINES
set to the string value"BAR"
. After the addition, we haveCPPDEFINES = ['BAR', ('FOO', 2)]
.processDefines
turns this into['BAR', 'FOO=2']
, and as output we get-DBAR -DFOO=2
.AppendUnique
to a single tupleCPPDEFINES
similar to 2) or 3) it fails with aTypeError
. For example, if trying to add a list:TypeError: can only concatenate list (not "tuple") to list
.It's possible that the most "consistent" approach would be to treat a lone tuple as "just an iterable" and not as a special case, and activate the special macro=value handling only if the tuple is seen as an element of an iterable (usually a list). Such a change probably also has the highest chance of affecting existing build systems.
The text was updated successfully, but these errors were encountered: