Fix problem where Java inner classes cannot cache

Generated files contained a '$' in filename and this blew up subst.
Situation arose because of a need to fetch the FS entry of the
source for finding permissions. Now we use the permissions of the
cached target to decide whether to chmod to add write permission,
this avoids the need to call File() on the source.

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

4 files changed, 107 insertions, 16 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index ecc82a4..087464b 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -98,7 +98,9 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       over 2100 lines.
     - Add a zipapp package of scons-local: can use SCons from a local
       file which does not need unpacking.
-
+    - Fix a problem (4.4 only) where a Java inner class could not be cached
+      because the emitted filename contained a '$' and ended up generating
+      a Python SyntaxError because is was passed through scons_subst().
 
 
 RELEASE 4.4.0 -  Sat, 30 Jul 2022 14:08:29 -0700
diff --git a/RELEASE.txt b/RELEASE.txt
index e9f2d02..76b51d1 100644
--- a/RELEASE.txt
+++ b/RELEASE.txt
@@ -69,6 +69,8 @@ FIXES
 - Fixed Issue #4275 - when outputting compilation db and TEMPFILE was in use, the compilation db would have
   command lines using the generated tempfile for long command lines, instead of the full command line for
   the compilation step for the source/target pair.
+- A refactor in the caching logic for version 4.4 left Java inner classes
+  failing with an exception when a CacheDir was enabled. This is now corrected.
 
 
 IMPROVEMENTS
diff --git a/SCons/CacheDir.py b/SCons/CacheDir.py
index 14e52ad..70c4f38 100644
--- a/SCons/CacheDir.py
+++ b/SCons/CacheDir.py
@@ -211,35 +211,42 @@ class CacheDir:
                                (self.requests, self.hits, self.misses, self.hit_ratio))
 
     @classmethod
-    def copy_from_cache(cls, env, src, dst):
+    def copy_from_cache(cls, env, src, dst) -> str:
+        """Copy a file from cache."""
         if env.cache_timestamp_newer:
             return env.fs.copy(src, dst)
         else:
             return env.fs.copy2(src, dst)
 
     @classmethod
-    def copy_to_cache(cls, env, src, dst):
+    def copy_to_cache(cls, env, src, dst) -> str:
+        """Copy a file to cache.
+
+        Just use the FS copy2 ("with metadata") method, except do an additional
+        check and if necessary a chmod to ensure the cachefile is writeable,
+        to forestall permission problems if the cache entry is later updated.
+        """
         try:
             result = env.fs.copy2(src, dst)
-            fs = env.File(src).fs
-            st = fs.stat(src)
-            fs.chmod(dst, stat.S_IMODE(st[stat.ST_MODE]) | stat.S_IWRITE)
+            st = stat.S_IMODE(os.stat(result).st_mode)
+            if not st | stat.S_IWRITE:
+                os.chmod(dst, st | stat.S_IWRITE)
             return result
         except AttributeError as ex:
             raise EnvironmentError from ex
 
     @property
-    def hit_ratio(self):
+    def hit_ratio(self) -> float:
         return (100.0 * self.hits / self.requests if self.requests > 0 else 100)
 
     @property
-    def misses(self):
+    def misses(self) -> int:
         return self.requests - self.hits
 
-    def is_enabled(self):
+    def is_enabled(self) -> bool:
         return cache_enabled and self.path is not None
 
-    def is_readonly(self):
+    def is_readonly(self) -> bool:
         return cache_readonly
 
     def get_cachedir_csig(self, node):
@@ -247,18 +254,21 @@ class CacheDir:
         if cachefile and os.path.exists(cachefile):
             return SCons.Util.hash_file_signature(cachefile, SCons.Node.FS.File.hash_chunksize)
 
-    def cachepath(self, node):
-        """
+    def cachepath(self, node) -> tuple:
+        """Return where to cache a file.
+
+        Given a Node, obtain the configured cache directory and
+        the path to the cached file, which is generated from the
+        node's build signature. If caching is not enabled for the
+        None, return a tuple of None.
         """
         if not self.is_enabled():
             return None, None
 
         sig = node.get_cachedir_bsig()
-
         subdir = sig[:self.config['prefix_len']].upper()
-
-        dir = os.path.join(self.path, subdir)
-        return dir, os.path.join(dir, sig)
+        cachedir = os.path.join(self.path, subdir)
+        return cachedir, os.path.join(cachedir, sig)
 
     def retrieve(self, node):
         """
diff --git a/test/Java/inner-cacheable-live.py b/test/Java/inner-cacheable-live.py
new file mode 100644
index 0000000..9f70291
--- /dev/null
+++ b/test/Java/inner-cacheable-live.py
@@ -0,0 +1,77 @@
+#!/usr/bin/env python
+#
+# MIT License
+#
+# Copyright The SCons Foundation
+#
+# Permission is hereby granted, free of charge, to any person obtaining
+# a copy of this software and associated documentation files (the
+# "Software"), to deal in the Software without restriction, including
+# without limitation the rights to use, copy, modify, merge, publish,
+# distribute, sublicense, and/or sell copies of the Software, and to
+# permit persons to whom the Software is furnished to do so, subject to
+# the following conditions:
+#
+# The above copyright notice and this permission notice shall be included
+# in all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
+# KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
+# WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+# OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+"""
+Test Java inner classes can be cached.  Requires a working JDK.
+
+Regression test: one iteration of CacheDir left it unable to deal
+with class names from the emitter which contained an embedded '$'.
+Led to error like:
+
+SyntaxError `invalid syntax (<string>, line 1)' trying to evaluate `$Inner.class'
+"""
+
+import TestSCons
+
+test = TestSCons.TestSCons()
+where_javac, java_version = test.java_where_javac()
+
+# Work around javac 1.4 not reporting its version:
+java_version = java_version or "1.4"
+
+# Skip this test as SCons doesn't (currently) predict the generated
+# inner/anonymous class generated .class files generated by gcj
+# and so will always fail.
+if test.javac_is_gcj:
+    test.skip_test('Test not valid for gcj (gnu java); skipping test(s).\n')
+
+test.write(
+    'SConstruct',
+    """
+env = Environment()
+env.CacheDir("cache")
+env.Java("classes", "source")
+"""
+    % locals(),
+)
+
+test.subdir('source')
+
+test.write(
+    ['source', 'Test.java'],
+    """\
+class Test { class Inner {} }
+""",
+)
+
+test.run(arguments='.')
+
+test.pass_test()
+
+# Local Variables:
+# tab-width:4
+# indent-tabs-mode:nil
+# End:
+# vim: set expandtab tabstop=4 shiftwidth=4: