renderdiff: fix update_golden.py

And fix other python bugs

RDIFF_ACCEPT_NEW_GOLDENS

test/renderdiff/src/golden_manager.py

@@ -16,6 +16,7 @@ import os
 import shutil
 import re
 import sys
+import tempfile
 
 from utils import execute, ArgParseImpl, mkdir_p
 
@@ -99,10 +100,11 @@ class GoldenManager:
     code, old_commit = execute(f'git log --format=%B -n 1', cwd=assets_dir)
     if tag and len(tag) > 0:
       old_commit += f'\nFILAMENT={tag}'
-      COMMIT_FILE = '/tmp/golden_commit.txt'
-      with open(COMMIT_FILE, 'w') as f:
+      with tempfile.NamedTemporaryFile('w', delete=False) as f:
         f.write(old_commit)
-      self._git_exec(f'commit --amend -F {COMMIT_FILE}')
+        commit_file = f.name
+      self._git_exec(f'commit --amend -F {commit_file}')
+      os.remove(commit_file)
 
     # Do the actual merge
     self._git_exec(f'checkout main')
@@ -139,11 +141,11 @@ class GoldenManager:
           os.path.join(rdiff_dir, f))
         self._git_exec(f'add {os.path.join(GOLDENS_DIR, f)}')
 
-    TMP_GOLDEN_COMMIT_FILE = '/tmp/golden_commit.txt'
-
-    with open(TMP_GOLDEN_COMMIT_FILE, 'w') as f:
+    with tempfile.NamedTemporaryFile('w', delete=False) as f:
       f.write(commit_msg)
-    self._git_exec(f'commit -a -F {TMP_GOLDEN_COMMIT_FILE}')
+      tmp_golden_commit_file = f.name
+    self._git_exec(f'commit -a -F {tmp_golden_commit_file}')
+    os.remove(tmp_golden_commit_file)
     if push_to_remote and \
        (self.access_token_ or self.access_type_ == ACCESS_TYPE_SSH):
       self._git_exec(f'push -f origin {branch}')

test/renderdiff/src/update_golden.py

@@ -64,19 +64,19 @@ def _do_update(golden_manager, config):
 
 def _same_image_diffimg(diffimg_path, img1, img2):
   cmd = [diffimg_path, img1, img2]
-  try:
-    result = subprocess.run(cmd, capture_output=True, text=True)
-    output = result.stdout.strip()
-    if not output:
-      return False
+  result = subprocess.run(cmd, capture_output=True, text=True)
+  if result.returncode != 0:
+    raise RuntimeError(f"diffimg failed with return code {result.returncode}:\n{result.stderr}")
 
-    try:
-      res_json = json.loads(output)
-      return res_json.get('passed', False)
-    except json.JSONDecodeError:
-      return False
-  except Exception:
-    return False
+  output = result.stdout.strip()
+  if not output:
+    raise RuntimeError("diffimg produced no output")
+
+  try:
+    res_json = json.loads(output)
+    return res_json.get('passed', False)
+  except json.JSONDecodeError as e:
+    raise RuntimeError(f"Failed to parse diffimg output: {e}\nOutput: {output}")
 
 def _get_deletes_updates(update_dir, golden_dir, diffimg_path):
   ret_delete = []
@@ -93,13 +93,13 @@ def _get_deletes_updates(update_dir, golden_dir, diffimg_path):
     # However, update_golden typically only updates/adds based on the new render set.
     # But strict sync might imply deleting missing ones.
     # The original logic was: delete = list(base - new).
-    delete = list(base - new)
+    delete_files = list(base_files - new_files)
 
     # Files in new but not in base are definitely updates (additions)
-    update = list(new - base)
+    update_files = list(new_files - base_files)
 
     # Files in both need comparison
-    for fpath in base.intersection(new_files):
+    for fpath in base_files.intersection(new_files):
       base_fpath = os.path.join(golden_dir, fpath)
       new_fpath = os.path.join(update_dir, fpath)
 
@@ -110,10 +110,10 @@ def _get_deletes_updates(update_dir, golden_dir, diffimg_path):
           is_different = _file_as_str(new_fpath) != _file_as_str(base_fpath)
 
       if is_different:
-        update.append(fpath)
+        ret_update.append(fpath)
 
-    ret_update += update
-    ret_delete += delete
+    ret_update += update_files
+    ret_delete += delete_files
 
   return ret_delete, ret_update
 
@@ -179,7 +179,7 @@ if __name__ == "__main__":
   parser = ArgParseImpl()
   parser.add_argument('--branch', help='Branch of the golden repo to write to')
   parser.add_argument('--golden-repo-token', help='Access token for the golden repo')
-  parser.add_argument('--push-to-remote', action="store_true", help='Access token for the golden repo')
+  parser.add_argument('--push-to-remote', action="store_true", help='Push the golden repo changes to remote')
   parser.add_argument('--diffimg', help='Path to the diffimg tool',
                       default='./out/cmake-release/tools/diffimg/diffimg')
 

test/renderdiff/src/utils.py

@@ -18,6 +18,7 @@ import shutil
 import argparse
 import sys
 import pathlib
+import shlex
 
 def execute(cmd,
             cwd=None,
@@ -25,7 +26,7 @@ def execute(cmd,
             stdin=None,
             env=None,
             raise_errors=False):
-  in_env = os.environ
+  in_env = os.environ.copy()
   in_env.update(env if env else {})
   home = os.environ['HOME']
   if f'{home}/bin' not in in_env['PATH']:
@@ -45,11 +46,11 @@ def execute(cmd,
       'universal_newlines': True
   }
   if capture_output:
-    process = subprocess.Popen(cmd.split(' '), **kwargs)
+    process = subprocess.Popen(shlex.split(cmd), **kwargs)
     output, err_output = process.communicate()
     return_code = process.returncode
   else:
-    return_code = subprocess.call(cmd.split(' '), **kwargs)
+    return_code = subprocess.call(shlex.split(cmd), **kwargs)
 
   if return_code:
     # Error