Fix zip tool to respect ZIPCOMSTR if it's set after an Environment or Tool('zip') is initialized. (It works fine if set in Environment()) Previous PR was #3659 by Rocco Matano

3 files changed, 31 insertions, 18 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index bbd7ae1..08f0aa3 100755
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -8,10 +8,10 @@ NOTE: The 4.0.0 Release of SCons dropped Python 2.7 Support
 
 RELEASE  VERSION/DATE TO BE FILLED IN LATER
 
-      From John Doe:
-
-        - Whatever John Doe did.
-
+  From Rocco Matano:
+    - Fix Zip tool to respect ZIPCOMSTR. Previously all zip builder calls would yield something
+      like zip(["test.zip"], ["zip_scons.py"]) and ignore ZIPCOMSTR if ZIPCOM and ZIPCOMSTR
+      weren't set after the Environment/Tool is initialized. (Explained in PR #3659)
 
 RELEASE 4.0.1 - Mon, 16 Jul 2020 16:06:40 -0700
 
diff --git a/SCons/Tool/zip.py b/SCons/Tool/zip.py
index 23d540f..cbf2c16 100644
--- a/SCons/Tool/zip.py
+++ b/SCons/Tool/zip.py
@@ -42,7 +42,9 @@ import SCons.Util
 
 import zipfile
 
-zipcompression = zipfile.ZIP_DEFLATED
+zip_compression = zipfile.ZIP_DEFLATED
+
+
 def zip(target, source, env):
     compression = env.get('ZIPCOMPRESSION', 0)
     zf = zipfile.ZipFile(str(target[0]), 'w', compression)
@@ -52,19 +54,20 @@ def zip(target, source, env):
                 for fname in filenames:
                     path = os.path.join(dirpath, fname)
                     if os.path.isfile(path):
-
                         zf.write(path, os.path.relpath(path, str(env.get('ZIPROOT', ''))))
         else:
             zf.write(str(s), os.path.relpath(str(s), str(env.get('ZIPROOT', ''))))
     zf.close()
 
-zipAction = SCons.Action.Action(zip, varlist=['ZIPCOMPRESSION'])
+# Fix PR #3569 - If you don't specify ZIPCOM and ZIPCOMSTR when creating
+# env, then it will ignore ZIPCOMSTR set afterwards.
+zipAction = SCons.Action.Action(zip, "$ZIPCOMSTR", varlist=['ZIPCOMPRESSION'])
 
-ZipBuilder = SCons.Builder.Builder(action = SCons.Action.Action('$ZIPCOM', '$ZIPCOMSTR'),
-                                   source_factory = SCons.Node.FS.Entry,
-                                   source_scanner = SCons.Defaults.DirScanner,
-                                   suffix = '$ZIPSUFFIX',
-                                   multi = 1)
+ZipBuilder = SCons.Builder.Builder(action=SCons.Action.Action('$ZIPCOM', '$ZIPCOMSTR'),
+                                   source_factory=SCons.Node.FS.Entry,
+                                   source_scanner=SCons.Defaults.DirScanner,
+                                   suffix='$ZIPSUFFIX',
+                                   multi=1)
 
 
 def generate(env):
@@ -75,12 +78,13 @@ def generate(env):
         bld = ZipBuilder
         env['BUILDERS']['Zip'] = bld
 
-    env['ZIP']        = 'zip'
-    env['ZIPFLAGS']   = SCons.Util.CLVar('')
-    env['ZIPCOM']     = zipAction
-    env['ZIPCOMPRESSION'] =  zipcompression
-    env['ZIPSUFFIX']  = '.zip'
-    env['ZIPROOT']    = SCons.Util.CLVar('')
+    env['ZIP'] = 'zip'
+    env['ZIPFLAGS'] = SCons.Util.CLVar('')
+    env['ZIPCOM'] = zipAction
+    env['ZIPCOMPRESSION'] = zip_compression
+    env['ZIPSUFFIX'] = '.zip'
+    env['ZIPROOT'] = SCons.Util.CLVar('')
+
 
 def exists(env):
     return True
diff --git a/test/ZIP/ZIPCOMSTR.py b/test/ZIP/ZIPCOMSTR.py
index af9ba57..43566fb 100644
--- a/test/ZIP/ZIPCOMSTR.py
+++ b/test/ZIP/ZIPCOMSTR.py
@@ -42,12 +42,21 @@ env = Environment(tools=['zip'],
                   ZIPCOM = r'%(_python_)s mycompile.py zip $TARGET $SOURCES',
                   ZIPCOMSTR = 'Zipping $TARGET from $SOURCE')
 env.Zip('aaa.zip', 'aaa.in')
+
+# Issue explained in PR #3569 - setting ZIPCOM/ZIPCOMSTR after env initialization
+# is ignored and yields zip() instead of desired ZIPCOMSTR>
+env2 = Environment(tools=['zip'])
+env2['ZIPCOM'] = r'%(_python_)s mycompile.py zip $TARGET $SOURCES'
+env2['ZIPCOMSTR']="TESTING ONE TWO THREE $TARGET from $SOURCE"
+env2.Zip('aaa2.zip', 'aaa.in')
+
 """ % locals())
 
 test.write('aaa.in', 'aaa.in\n/*zip*/\n')
 
 test.run(stdout = test.wrap_stdout("""\
 Zipping aaa.zip from aaa.in
+TESTING ONE TWO THREE aaa2.zip from aaa.in
 """))
 
 test.must_match('aaa.zip', "aaa.in\n")