Split out CPPDEFINES handling in Append methods

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 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 #4254
Fixes #3876

Signed-off-by: Mats Wichmann <mats@linux.com>

CHANGES.txt

@@ -133,6 +133,17 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       Also added "unique" keyword, to control whether a library is added
       or not if it is already in the $LIBS construction var in the
       configure context. (issue #2768).
+    - Special-case handling for CPPDEFINES in the Append/Prepend routines
+      split out into its own function to simplify the remaining code and
+      fix problems. The e2e test test/CPPDEFINES/append.py is expanded
+      to cover missed cases, and AppendUnique no longer mismatches with
+      what Append does (#3876).  Inconsistent handling of tuples to specify
+      macro=value outputs is also cleaned up (#4254). The special handling
+      now also works for Prepend/PrependUnique, and a corresponding test
+      test/CPPDEFINES/prepend.py was added to verify the behavior.
+      SCons used to sort keys set or appended via a dict type, in order to
+      assure order of commandline flags did not change across runs. This
+      behavior has been dropped since Python now assures consistent dict order.
 
 
 RELEASE 4.4.0 -  Sat, 30 Jul 2022 14:08:29 -0700
@@ -295,16 +306,16 @@ RELEASE 4.4.0 -  Sat, 30 Jul 2022 14:08:29 -0700
     - Ninja:Added user configurable setting of ninja depfile format via NINJA_DEPFILE_PARSE_FORMAT.
       Now setting NINJA_DEPFILE_PARSE_FORMAT to [msvc,gcc,clang] can force the ninja expected
       format. Compiler tools will also configure the variable automatically.
-    - Ninja: Made ninja tool force the ninja file as the only target. 
+    - Ninja: Made ninja tool force the ninja file as the only target.
     - Ninja: Improved the default targets setup and made sure there is always a default target for
       the ninja file, which excludes targets that start and stop the daemon.
     - Ninja: Update ninja tool so targets passed to SCons are propagated to ninja when scons
       automatically executes ninja.
-    - Small refactor of scons daemons using a shared StateInfo class for communication 
+    - Small refactor of scons daemons using a shared StateInfo class for communication
       between the scons interactive thread and the http server thread. Added error handling
       for scons interactive failing to startup.
     - Ninja: Updated ninja scons daemon scripts to output errors to stderr as well as the daemon log.
-    - Ninja: Fix typo in ninja scons daemon startup which causes ConnectionRefusedError to not retry 
+    - Ninja: Fix typo in ninja scons daemon startup which causes ConnectionRefusedError to not retry
     - Added SHELL_ENV_GENERATORS construction variable. This variable should be set to a list
       (or an iterable) which contains functions to be called in order
       when constructing the execution environment (Generally this is the shell environment
@@ -546,7 +557,7 @@ RELEASE 4.2.0 - Sat, 31 Jul 2021 18:12:46 -0700
     - Fix Issue #3906 - `IMPLICIT_COMMAND_DEPENDENCIES` was not properly disabled when
       set to any string value (For example ['none','false','no','off'])
       Also previously 'All' wouldn't have the desired affect.
-      
+
   From Ivan Kravets:
     - Provide a custom argument escape function for `TempFileMunge` using a new
       `TEMPFILEARGESCFUNC` variable. Useful if you need to apply extra operations on

RELEASE.txt

@@ -51,7 +51,12 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY
   which controls whether to add the library if it is already in the $LIBS
   list. This brings the library-adding functionality in Configure in line
   with the regular Append, AppendUnique, Prepend and PrependUnique methods.
-
+- CPPDEFINES values added via a dictionary type are longer sorted by
+  key. This used to be required to maintain a consistent order of
+  commandline arguments between SCons runs, but meant macros were not
+  always emitted in the order entered. Sorting is no longer required
+  after Python interpreter improvements.  There might be a one-time 
+  rebuild of targets that involved such sorted keys in their actions.
 
 FIXES
 -----
@@ -90,7 +95,17 @@ FIXES
   actual build would not see the file as modified. Leading to incorrect incremental builds.
   Now configure checks now clear node info for non conftest nodes, so they will be re-evaluated for
   the real taskmaster run when the build commences.
-
+- Inconsistent behavior of adding CPPDEFINES values via tuple arguments
+  has been cleaned up (problems described in issue #4254). In order to
+  achieve consistency, a rule for for adding macros-with-values via a
+  tuple (or list) is now enforced: they need to be entered as members
+  of a containing sequence. ["NAME", "VALUE"] and ("NAME", "VALUE")
+  are now treated the same (two individual macro names), rather than
+  the latter sometimes, but not always, being handled as a single
+  NAME=VALUE macro. Use a construct like [("NAME", "VALUE")] to get the
+  macro-with-value behavior.
+- Handling of CPPDEFINES macros via Prepend and PrependUnique now works
+  (previously this was special-cased only for Append and AppendUnique).
 
 IMPROVEMENTS
 ------------
@@ -133,12 +148,17 @@ DOCUMENTATION
 - Updated the User Guide chapter on installation: modernized the notes
   on Python installs, SCons installs, and having multiple SCons versions
   present on a single system.
+<<<<<<< HEAD
 - Updated the User Guide chapter on variant directories with more
   explanation, and the introduction of terms like "out of tree" that
   may help in forming a mental model.
 - Additional explanations for MSVSProject and MSVSSolution builders.
 - Updated MSVC documentation - adds "version added" annotations on recently
   added construction variables and provides a version-mapping table.
+=======
+- Added further details in the documentation of Append and related methods
+  on the special handling of CPPDEFINES.
+>>>>>>> d9a82d681 (Split out CPPDEFINES handling in Append methods)
 
 DEVELOPMENT
 -----------

SCons/Defaults.py

@@ -36,6 +36,7 @@
 import stat
 import sys
 import time
+from collections import deque
 
 import SCons.Action
 import SCons.Builder
@@ -46,7 +47,7 @@
 import SCons.Scanner.Dir
 import SCons.Subst
 import SCons.Tool
-import SCons.Util
+from SCons.Util import is_List, is_String, is_Tuple, is_Dict, flatten
 
 # A placeholder for a default Environment (for fetching source files
 # from source code management systems and the like).  This must be
@@ -166,7 +167,7 @@ def get_paths_str(dest) -> str:
     def quote(arg):
         return f'"{arg}"'
 
-    if SCons.Util.is_List(dest):
+    if is_List(dest):
         elem_strs = [quote(d) for d in dest]
         return f'[{", ".join(elem_strs)}]'
     else:
@@ -202,11 +203,11 @@ def chmod_func(dest, mode) -> None:
     """
     from string import digits
     SCons.Node.FS.invalidate_node_memos(dest)
-    if not SCons.Util.is_List(dest):
+    if not is_List(dest):
         dest = [dest]
-    if SCons.Util.is_String(mode) and 0 not in [i in digits for i in mode]:
+    if is_String(mode) and 0 not in [i in digits for i in mode]:
         mode = int(mode, 8)
-    if not SCons.Util.is_String(mode):
+    if not is_String(mode):
         for element in dest:
             os.chmod(str(element), mode)
     else:
@@ -244,7 +245,7 @@ def chmod_func(dest, mode) -> None:
 
 def chmod_strfunc(dest, mode) -> str:
     """strfunction for the Chmod action function."""
-    if not SCons.Util.is_String(mode):
+    if not is_String(mode):
         return f'Chmod({get_paths_str(dest)}, {mode:#o})'
     else:
         return f'Chmod({get_paths_str(dest)}, "{mode}")'
@@ -272,10 +273,10 @@ def copy_func(dest, src, symlinks=True) -> int:
     """
 
     dest = str(dest)
-    src = [str(n) for n in src] if SCons.Util.is_List(src) else str(src)
+    src = [str(n) for n in src] if is_List(src) else str(src)
 
     SCons.Node.FS.invalidate_node_memos(dest)
-    if SCons.Util.is_List(src):
+    if is_List(src):
         # this fails only if dest exists and is not a dir
         try:
             os.makedirs(dest, exist_ok=True)
@@ -322,7 +323,7 @@ def delete_func(dest, must_exist=False) -> None:
     unless *must_exist* evaluates false (the default).
     """
     SCons.Node.FS.invalidate_node_memos(dest)
-    if not SCons.Util.is_List(dest):
+    if not is_List(dest):
         dest = [dest]
     for entry in dest:
         entry = str(entry)
@@ -348,7 +349,7 @@ def delete_strfunc(dest, must_exist=False) -> str:
 def mkdir_func(dest) -> None:
     """Implementation of the Mkdir action function."""
     SCons.Node.FS.invalidate_node_memos(dest)
-    if not SCons.Util.is_List(dest):
+    if not is_List(dest):
         dest = [dest]
     for entry in dest:
         os.makedirs(str(entry), exist_ok=True)
@@ -372,7 +373,7 @@ def move_func(dest, src) -> None:
 def touch_func(dest) -> None:
     """Implementation of the Touch action function."""
     SCons.Node.FS.invalidate_node_memos(dest)
-    if not SCons.Util.is_List(dest):
+    if not is_List(dest):
         dest = [dest]
     for file in dest:
         file = str(file)
@@ -433,7 +434,7 @@ def _concat_ixes(prefix, items_iter, suffix, env):
     prefix = str(env.subst(prefix, SCons.Subst.SUBST_RAW))
     suffix = str(env.subst(suffix, SCons.Subst.SUBST_RAW))
 
-    for x in SCons.Util.flatten(items_iter):
+    for x in flatten(items_iter):
         if isinstance(x, SCons.Node.FS.File):
             result.append(x)
             continue
@@ -479,16 +480,16 @@ def _stripixes(prefix, itms, suffix, stripprefixes, stripsuffixes, env, c=None):
         else:
             c = _concat_ixes
 
-    stripprefixes = list(map(env.subst, SCons.Util.flatten(stripprefixes)))
-    stripsuffixes = list(map(env.subst, SCons.Util.flatten(stripsuffixes)))
+    stripprefixes = list(map(env.subst, flatten(stripprefixes)))
+    stripsuffixes = list(map(env.subst, flatten(stripsuffixes)))
 
     stripped = []
     for l in SCons.PathList.PathList(itms).subst_path(env, None, None):
         if isinstance(l, SCons.Node.FS.File):
             stripped.append(l)
             continue
 
-        if not SCons.Util.is_String(l):
+        if not is_String(l):
             l = str(l)
 
         for stripprefix in stripprefixes:
@@ -511,49 +512,49 @@ def _stripixes(prefix, itms, suffix, stripprefixes, stripsuffixes, env, c=None):
 
 
 def processDefines(defs):
-    """process defines, resolving strings, lists, dictionaries, into a list of
-    strings
+    """Return list of strings for preprocessor defines from *defs*.
+
+    Resolves all the different forms CPPDEFINES can be assembled in.
+    Any prefix/suffix is handled elsewhere (usually :func:`_concat_ixes`).
     """
-    if SCons.Util.is_List(defs):
-        l = []
-        for d in defs:
-            if d is None:
+    dlist = []
+    if is_List(defs):
+        for define in defs:
+            if define is None:
                 continue
-            elif SCons.Util.is_List(d) or isinstance(d, tuple):
-                if len(d) >= 2:
-                    l.append(str(d[0]) + '=' + str(d[1]))
+            elif is_List(define) or is_Tuple(define):
+                if len(define) >= 2 and define[1] is not None:
+                    # TODO: do we need to quote define[1] if it contains space?
+                    dlist.append(str(define[0]) + '=' + str(define[1]))
                 else:
-                    l.append(str(d[0]))
-            elif SCons.Util.is_Dict(d):
-                for macro, value in d.items():
+                    dlist.append(str(define[0]))
+            elif is_Dict(define):
+                for macro, value in define.items():
                     if value is not None:
-                        l.append(str(macro) + '=' + str(value))
+                        # TODO: do we need to quote value if it contains space?
+                        dlist.append(str(macro) + '=' + str(value))
                     else:
-                        l.append(str(macro))
-            elif SCons.Util.is_String(d):
-                l.append(str(d))
+                        dlist.append(str(macro))
+            elif is_String(define):
+                dlist.append(str(define))
             else:
-                raise SCons.Errors.UserError("DEFINE %s is not a list, dict, string or None." % repr(d))
-    elif SCons.Util.is_Dict(defs):
-        # The items in a dictionary are stored in random order, but
-        # if the order of the command-line options changes from
-        # invocation to invocation, then the signature of the command
-        # line will change and we'll get random unnecessary rebuilds.
-        # Consequently, we have to sort the keys to ensure a
-        # consistent order...
-        l = []
-        for k, v in sorted(defs.items()):
-            if v is None:
-                l.append(str(k))
+                raise SCons.Errors.UserError(
+                    f"DEFINE {d!r} is not a list, dict, string or None."
+                )
+    elif is_Dict(defs):
+        for macro, value in defs.items():
+            if value is None:
+                dlist.append(str(macro))
             else:
-                l.append(str(k) + '=' + str(v))
+                dlist.append(str(macro) + '=' + str(value))
     else:
-        l = [str(defs)]
-    return l
+        dlist.append(str(defs))
+
+    return dlist
 
 
 def _defines(prefix, defs, suffix, env, target=None, source=None, c=_concat_ixes):
-    """A wrapper around _concat_ixes that turns a list or string
+    """A wrapper around :func:`_concat_ixes` that turns a list or string
     into a list of C preprocessor command-line definitions.
     """