fix symbol table add changing value of existing variable

Noticed my symbol table add() method was always creating a new entry vs
updating the entry that existed. Now will update the Entry's value.

Added some additional complex rules to handle Command Line variables.

Also snuck in a change to remove some dead code that was confusing my
greps.

Author: linuxlizard

pymake/pargs.py

@@ -113,15 +113,16 @@ def __init__(self):
 
     def __str__(self):
         # useful for debugging the sub-make
-        s = ""
-        s += "-B " if self.always_make else ""
-        s += "-C %s " % self.directory if self.directory else ""
-        s += "-d " if self.debug else ""
-        s += "-f %s " % self.filename if self.filename else ""
-        s += "-n " if self.dry_run else ""
-        s += "-s " if self.silent else ""
-        s += " ".join(self.argslist)
-        return s
+        a = [
+            "-B" if self.always_make else "",
+            "-C %s" % self.directory if self.directory else "",
+            "-d" if self.debug else "",
+            "-f %s" % self.filename if self.filename else "",
+            "-n" if self.dry_run else "",
+            "-s" if self.silent else "",
+            *self.argslist
+        ]
+        return " ".join( [s for s in a if s] )
 
 def _parse_debug_flags(s):
     flags = s.split(',')

pymake/parser.py

@@ -752,116 +752,6 @@ def parse_directive(directive_vstr, virt_line, vline_iter):
 
     return lut[str(directive_vstr)](directive_vstr, virt_line, vline_iter)
 
-def parse_expression(expr, virt_line, vline_iter):
-    # davep 20241123 ; obsolete 
-    assert 0, "TODO"
-
-    # This is a second pass through an Expression.
-    # An Expression could be something like:
-    #   $(info blah blah)  # fn call ; most are invalid in standalone context
-    #   export 
-    #   export something
-    #   define foo   # start of multi-line variable
-
-    logger.debug("parse %s", expr.__class__.__name__)
-#    if get_line_number(expr) > 151:
-#        breakpoint()
-    assert isinstance(expr,Expression), type(expr)
-
-    # If we do find a directive, we'll wind up re-parsing the entire line as a
-    # directive. Unnecessary and ugly but I first tried to handle directives
-    # before assignment and rules which didn't work (too much confusion in
-    # handling the case of directive names as as rule or assignments). So I'm
-    # parsing the line first, determining if it's a rule or statement or
-    # expression. If expression, look for a directive string, then connect
-    # to the original Directive handling code here.
-
-    assign_expr = None
-    if isinstance(expr, AssignmentExpression):
-        # weird case showing GNU Make's lack of reserved words and the
-        # sloppiness of my grammar tokenparser.  
-        # define xyzzy :=    <-- multi-line variable masquerading as an Assignment
-        # ifdef := 123  <-- totally legal but I want to throw a warning
-        # export CC=gcc  <-- looks like an assignment
-        # Need to dig into the assignment to get the LHS.
-        assign_expr = expr
-        expr = expr.token_list[0]
-    
-    # We're only interested in Directives at this point. A Directive will be
-    # inside string Literal. 
-    #
-    # Leave anything else alone. Invalid context function calls will error out
-    # during execute().  For example,  $(subst ...) alone on a line will error
-    # with "*** missing separator. Stop."
-
-    # If the first token isn't a Literal, we're done.
-    tok = expr.token_list[0]
-    if not isinstance(tok, Literal):
-        return assign_expr if assign_expr else expr
-
-    # seek_directive() needs a character iterator 
-    viter = ScannerIterator(tok.string, tok.string.get_pos()[0])
-
-    # peek at the first character of the line. If it's a <tab> aka recipeprefix then 
-    # WOW does our life get complicated.  GNU Make allows directives (with a
-    # couple exceptions) to be recipeprefix'd. 
-    # TODO not handling recipeprefix yet
-    first_vchar = viter.lookahead()
-
-    directive_vstr = seek_directive(viter)
-    if not directive_vstr:
-        # nope, not a directive. Ignore this expression and let execute figure it out
-        return assign_expr if assign_expr else expr
-
-    # at this point, we definitely have some sort of directive.
-    assert viter.is_empty()
-
-    # delete the whitespace token after the directive if it exists
-    if len(expr.token_list) > 1 and expr.token_list[1].is_whitespace():
-        del expr.token_list[1]
-        
-    dir_str = str(directive_vstr)
-
-    if assign_expr:
-        # If we've peeked into an assignment and decided this is re-using a
-        # directive name as an assign, throw a warning about using directive
-        # name in a weird context.
-        #
-        # yet another corner case of totally legit directive syntax:
-        # define = foo
-        # define = <nothing>
-        # export CC=gcc
-        # override CC=xcc
-        # override define foo=
-
-        if dir_str in ("define", "export", "override") and len(expr.token_list) > 1:
-            # These directives can validly be an assignment so parse them as
-            # a directive.  The len() above checks that there's multiple
-            # symbols on the LHS before the assignment operator.
-
-            # extract the directive from the expression
-            # We want to throw away the first Literal containing the directive
-            # expr is a ref into assign_expr.token_list[0]
-            # so I should be able to delete expr.token_list[0] and kill the directive literal
-            assert assign_expr.token_list[0].token_list[0] is expr.token_list[0]
-
-            del expr.token_list[0]
-
-            dir_ = parse_directive(assign_expr, directive_vstr, virt_line, vline_iter)
-        else:
-            # GNU Make doesn't warn like this (pats self on back).
-            logger.warning("re-using a directive name \"%s\" in an assignment at %r", dir_str, assign_expr.get_pos())
-            return assign_expr
-    else:
-        # extract the directive from the expression
-        # We want to throw away the first Literal containing the directive
-        del expr.token_list[0] 
-
-        dir_ = parse_directive(expr, directive_vstr, virt_line, vline_iter)
-
-#    print("parse directive=%s" % dir_.makefile())
-    return dir_
-
 def parse_rule(vchar_scanner):
     lhs = tokenizer.tokenize_rule(vchar_scanner)
     if lhs is None:

pymake/pymake.py

@@ -639,6 +639,7 @@ def execute(makefile, args):
     return exit_status["error"] if exit_code else exit_status["success"] 
 
 def _run_it(args):
+    logger.debug("run_it args=\"%s\"", args)
     # -C option
     if args.directory:
         os.chdir(os.path.join(*args.directory))

pymake/symbol.py

@@ -144,8 +144,7 @@ class Literal(Symbol):
     def __init__(self, vstring):
         # catch bugs where I create empty Literals
         if not len(vstring):
-            breakpoint()
-        assert len(vstring)
+            raise InternalError("attempt to create empty vstring")
 
         # cache the literal string
         self._str = str(vstring)
@@ -349,7 +348,12 @@ def assign(lhs, op, rhs, symbol_table, flags=0):
         pos = lhs.get_pos()
 
         logger.debug("assignment rhs=%s", rhs)
-#        breakpoint()
+
+        if flags & AssignmentExpression.FLAG_OVERRIDE:
+            raise NotImplementedError("override")
+
+        if flags & AssignmentExpression.FLAG_PRIVATE:
+            raise NotImplementedError("private")
 
         if op_str == "?=":
             symbol_table.maybe_add(key, rhs, pos)
@@ -422,7 +426,6 @@ def add_modifiers(self, modifier_list):
                 self.modifier_flags |= self.FLAG_OVERRIDE
             else:
                 assert 0, s
-            
         self.sanity()
 
     def __str__(self):

pymake/symtable.py

@@ -82,6 +82,7 @@ def value(self):
 
     def set_value(self, value, pos):
         # TODO add a stack where the values get changed
+        logger.debug("overwrite value name=%s at pos=%r", self.name, pos)
         self.pos = pos
         self._value = value
 
@@ -151,9 +152,21 @@ def sanity(self):
 # -- GNU Make manual  Version 4.3 Jan 2020
 class DefaultEntry(Entry):
     origin = "default"
-    never_export = True
 
-class CallbackEntry(DefaultEntry):
+# An Immutable Entry is one where we cannot change the value BUT we can
+# completely change the entry. For example, .VARIABLES has a special meaning in
+# Make (a string containing names of all the defined variables) BUT we can do something bone-headed like:
+#
+# .VARIABLES:=foo  # kill the .VARIABLES variable
+#
+# When am Immutable Entry is updated, the set_value() will throw an exception.
+# The Symbol Table add() method will then create a new entry instead.
+#
+class ImmutablemixIn:
+    def set_value(self, value, pos):
+        raise ValueError("cannot update CallbackEntry name=%s" % self.name)
+
+class CallbackEntry(ImmutablemixIn, DefaultEntry):
     # Some built-in variables have complex requirements that can't be stored as
     # a simple string in the symbol table. For example .VARIABLES returns a
     # list of the variables currently defined.
@@ -167,12 +180,13 @@ def __init__(self, name, callback_fn):
     def eval(self, symbol_table):
         return self._value(symbol_table)
 
+
 # Environment Variables are higher precedence than File variables.  But Command
 # Line args are higher precedence than Environment Variables.
 # "By default, only variables that came from the environment or the
 # command line are passed to recursive invocations."
 # -- GNU Make manual  Version 4.3 Jan 2020
-class EnvVarEntry(Entry):
+class EnvVarEntry(ImmutablemixIn, Entry):
     origin = "environment"
 
     def __init__(self, name, value):
@@ -188,16 +202,13 @@ def __init__(self, name, value):
 # "By default, only variables that came from the environment or the
 # command line are passed to recursive invocations."
 # -- GNU Make manual  Version 4.3 Jan 2020
-class CommandLineEntry(Entry):
+class CommandLineEntry(ImmutablemixIn, Entry):
     origin = "command line"
 
     def __init__(self, name, value):
         super().__init__(name,value)
         self.set_export(Export.SUBMAKE)
 
-    def set_value(self, value, pos):
-        pass
-
 
 class AutomaticEntry(Entry):
     origin = "automatic"
@@ -312,29 +323,49 @@ def add(self, name, value, pos=None):
 
         try:
             entry = self.symbols[name]
-            logger.debug("overwrite value name=%s at pos=%r", entry.name, pos)
         except KeyError:
-            entry = None
-
-        # if we didn't find it, make it
-        if self.command_line_flag:
-            # this flag is a weird hack to support vars created by eval()'ing
-            # expressions from the command line
-            new_entry = CommandLineEntry(name, value)
-            # command line vars are always exported until explicitly unexported
+            entry = None 
+
+        if entry is None:
+            # easy case: if we didn't find it, make it
+            if self.command_line_flag:
+                # this flag is a weird hack to support vars created by eval()'ing
+                # expressions from the command line
+                # - command line vars are always exported until explicitly
+                #   marked 'unexport'
+                # - command line vars cannot be overwritten by file variables
+                #   except by the 'override' directive (not yet implemented)
+                entry = CommandLineEntry(name, value)
+            else:
+                if pos and pos[0] == "@defaults":
+                    entry = DefaultEntry(name, value, pos)
+                else:
+                    entry = FileEntry(name, value, pos)
+                entry.set_export(self.export_default_value)
+
+            self._add_entry(entry)
         else:
-            # command line > file
-            if isinstance(entry,CommandLineEntry):
-                return
-
-            new_entry = FileEntry(name, value, pos)
-            new_entry.set_export(self.export_default_value)
-
-        # XXX not sure read-only is a good idea yet
-#        if entry.read_only:
-#            raise PermissionDenied(name)
-
-        self._add_entry(new_entry)
+            overwrite = False
+            try:
+                entry.set_value(value, pos)
+            except ValueError:
+                overwrite = True
+
+            if overwrite:
+                # Found an immutable variable that refuses to be overwritten.
+                # So (maybe) create a new one.
+                overwrite_entry = None
+                if self.command_line_flag:
+                    # command line entry can happily override command line entry
+                    overwrite_entry = CommandLineEntry(name, value)
+                else:
+                    # do not overwrite a command line entry when we're
+                    # no longer processing command line variables
+                    if not isinstance(entry, CommandLineEntry):
+                        overwrite_entry = FileEntry(name, value, pos)
+
+                if overwrite_entry:
+                    self._add_entry(overwrite_entry)
 
     def add_automatic(self, name, value, pos):
         entry = AutomaticEntry(name, value, pos)

tests/test_assign.py

@@ -1,3 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2014-2024 David Poole davep@mbuf.com david.poole@ericsson.com
+
 import run
 
 # Test all the ways we can assign a variable
@@ -37,3 +40,14 @@ def test_add():
     expect = "FOO=foo bar"
     run_test(makefile, expect)
 
+def test_update():
+    makefile = """
+FOO:=foo
+FOO:=$(FOO) foo
+FOO:=bar $(FOO)
+
+@:; @echo $(FOO)
+"""
+    expect = "bar foo foo"
+    run_test(makefile, expect)
+

tests/test_export.py

@@ -264,12 +264,35 @@ def test_specific_unexport():
     expect=("gcc","no CFLAGS for you")
     run_test(makefile,expect)
 
-@pytest.mark.skip(reason="FIXME")
-def test_circular_export_bug():
+def test_circular_export():
     makefile="""
-VERSION=$(shell cat /etc/os-release)
-export VERSION
-@:;@:
+FOO=$(shell echo foo)
+export FOO
+@:; @printenv FOO
 """
-    expect = ("",)
+    expect = ("foo",)
     run_test(makefile,expect)
+
+def test_export_commandline_var():
+    # command line variable assignments always override export status of file
+    # variables
+    makefile="""
+export FOO:=foo
+@:; @echo $(FOO)
+"""
+    expect = ("bar",)
+    run_test(makefile, expect, extra_args=("FOO=bar",))
+    
+
+def test_export_commandline_var_explicit():
+    # command line variable assignments always override export status of file
+    # variables
+    makefile="""
+export FOO:=foo
+@:; @echo $(FOO)
+"""
+    expect = ("bar",)
+    # use explicity assign instead of recursive assign
+    run_test(makefile, expect, extra_args=("FOO:=bar",))
+    
+

tests/test_origin.py

@@ -87,3 +87,13 @@ def test_command_line():
     output = run.gnumake_string(makefile, extra_args=("FOO=BAR",))
     assert output.strip() == "command line", output
 
+def test_default():
+    makefile="""
+ifneq ($(origin CC),default)
+$(error CC origin should be 'default' not '$(origin CC)')
+endif
+
+@:;@:
+"""
+    run.simple_test(makefile)
+