renderdiff: enable update goldens on commit merge (#8771)

test/renderdiff/README.md

@@ -7,11 +7,72 @@ machines. To perform software rasterization, these scripts are centered around [
 rasterizers, but nothing bars us from using another rasterizer like [SwiftShader]. Additionally,
 we should be able to use GPUs where available (though this is more of a future work).
 
-The script `run.py` contains the core logic for taking input parameters (such as the test
-description file) and then running gltf_viewer to produce the results.
+The script `render.py` contains the core logic for taking input parameters (such as the test
+description file) and then running gltf_viewer to produce the renderings.
 
 In the `test` directory is a list of test descriptions that are specified in json.  Please see
 `sample.json` to parse the structure.
 
+## Running the test locally
+ - To run the same presbumit as [`test-renderdiff`][presubmit-renderdiff], you can do
+   ```
+bash test/renderdiff/test.sh
+   ```
+ - This script will generate the renderings based on the current state of your repo.
+   Additionally, it will also compare the generated images with corresponding images in the
+   golden repo.
+ - To just render without running the test, you could use the following script
+   ```
+bash test/renderdiff/generate.sh
+   ```
+
+## Update the golden images
+The golden images are stored in a github repository: https://github.com/google/filament-assets.
+Filament team members should have access to write to the repository. A typical flow for updating
+the goldens is to upload your changed images into **branch** of `filament-assets`. This branch is
+paired with a PR or commit on the `filament` repo.
+
+As an example, imagine I am working on a PR, and I've uploaded my change, which is in a branch
+called `my-pr-branch`, to `filament`. This PR requires updating the golden. We would do it
+in the following fashion
+
+### Using a script to update the golden repo
+
+ - Run interactive mode in the `update_golden.py` script.
+   ```
+python3 test/renderdiff/src/update_golden.py
+   ```
+ - This will guide you through a series of steps to push the changes to a remote branch
+   on `filament-assets`.
+
+### Manually updating the golden repo
+
+ - Check out the golden repo
+   ```
+git clone git@github.com:google/filament-assets.git
+   ```
+ - Create a branch on the golden repo
+   ```
+cd filament-assets
+git switch -c my-pr-branch-golden
+   ```
+ - Copy the new images to their appropriate place in `filament-assets`
+ - Push the `filament-assets` working branch to remote
+   ```
+git push origin my-pr-branch-golden
+   ```
+ - In the commit message of your working branch on `filament`, add the following line
+   ```
+RDIFF_BBRANCH=my-pr-branch-golden
+   ```
+### Manually updating the golden repo
+
+Doing the above has multiple effects:
+ - The presubmit test [`test-renderdiff`][presubmit-renderdiff] will test against the provided
+   branch of the golden repo (i.e. `my-pr-branch-golden`).
+ - If the PR is merged, then there is another workflow that will merge `my-pr-branch-golden` to
+   the `main` branch of the golden repo.
+
 [Mesa]: https://docs.mesa3d.org
-[SwiftShader]: https://github.com/google/swiftshader
+[SwiftShader]: https://github.com/google/swiftshader
+[presubmit-renderdiff]: https://github.com/google/filament/blob/e85dfe75c86106a05019e13ccdbef67e030af675/.github/workflows/presubmit.yml#L118

test/renderdiff/src/commit_msg.py

@@ -12,11 +12,15 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from utils import execute
+import sys
+import re
 
-def get_last_commit():
-  res, o = execute('git log -1')
-  commit, author, date, _, title, *desc = o.split('\n')
+from utils import execute, ArgParseImpl
+
+RDIFF_UPDATE_GOLDEN_STR = 'RDIFF_BRANCH'
+
+def _parse_commit(commit_str):
+  commit, author, date, _, title, *desc = commit_str.split('\n')
   commit = commit.split(' ')[1]
   title = title.strip()
 
@@ -30,28 +34,27 @@ def get_last_commit():
     desc
   )
 
-def sanitized_split(line, split_atom='\n'):
-  return list(
-    filter(
-      lambda x: len(x) > 0,
-      map(
-        lambda x: x.strip(),
-        line.split(split_atom)
-      )
-    )
-  )
-
-RDIFF_UPDATE_GOLDEN_STR = 'RDIFF_UPDATE_GOLDEN'
-
 if __name__ == "__main__":
-  RE_STR = f'{RDIFF_UPDATE_GOLDEN_STR}(?:S)?=[\[]?([a-zA-Z0-9,\s]+)[\]]?'
+  RE_STR = rf"{RDIFF_UPDATE_GOLDEN_STR}(?:S)?=[\[]?([a-zA-Z0-9,\s\-\/]+)[\]]?"
+
+  parser = ArgParseImpl()
+  parser.add_argument('--file', help='A file containing the commit message')
+  args, _ = parser.parse_known_args(sys.argv[1:])
+
+  if not args.file:
+    msg = sys.stdin.read()
+  else:
+    with open(args.file, 'r') as f:
+      msg = f.read()
 
   to_update = []
-  commit, title, description = get_last_commit()
+  commit, title, description = _parse_commit(msg)
   for line in description:
     m = re.match(RE_STR, line)
     if not m:
       continue
+    print(m.group(1))
+    exit(0)
 
-    to_update += sanitize_split(m.group(1).replace(',', ' '), ' ')
-  print(','.join(to_update))
+  # Always default to the main branch
+  print('main')

test/renderdiff/src/golden_manager.py

@@ -65,7 +65,7 @@ class GoldenManager:
     assets_dir = self._assets_dir()
     if not os.path.exists(assets_dir):
       execute(
-          f'git clone --depth=1 {self._get_repo_url()}',
+          f'git clone {self._get_repo_url()}',
           cwd=self.working_dir_,
           capture_output=False
       )
@@ -83,13 +83,26 @@ class GoldenManager:
     self._git_exec('rebase')
 
   def _git_exec(self, cmd):
-    execute(f'git {cmd}', cwd=self._assets_dir(), capture_output=False)
+    return execute(f'git {cmd}', cwd=self._assets_dir(), capture_output=False)
 
-  def merge_to_main(self, branch, push_to_remote=False):
+  # tag represent a hash in the filament repo that this merge is associated with
+  def merge_to_main(self, branch, tag, push_to_remote=False):
     self.update()
     assets_dir = self._assets_dir()
+
+    # Update commit message
+    self._git_exec(f'checkout {branch}')
+    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:
+        f.write(old_commit)
+      self._git_exec(f'commit --amend -F {COMMIT_FILE}')
+
+    # Do the actual merge
     self._git_exec(f'checkout main')
-    self._git_exec(f'merge --no-ff {branch}')
+    self._git_exec(f'merge --no-ff --no-edit {branch}')
     if push_to_remote and \
        (self.access_token_ or self.access_type_ == ACCESS_TYPE_SSH):
       self._git_exec(f'push origin main')
@@ -109,7 +122,7 @@ class GoldenManager:
       self._git_exec(f'add {GOLDENS_DIR}')
     else:
       for f in deletes:
-        self._git_exec(f'remove {os.path.join(GOLDENS_DIR, f)}')
+        self._git_exec(f'rm {os.path.join(GOLDENS_DIR, f)}')
       for f in updates:
         shutil.copy2(
           os.path.join(src_dir, f),

test/renderdiff/src/update_golden.py

@@ -34,7 +34,7 @@ CONFIG_NEW_SRC_DIR = 'goldens_dir'
 CONFIG_GOLDENS_BRANCH = 'goldens_branch'
 CONFIG_GOLDENS_UPDATES = 'goldens_updates'
 CONFIG_GOLDENS_DELETES = 'goldens_deletes'
-CONFIG_AUTO_COMMIT = 'auto-commit'
+CONFIG_PUSH_TO_REMOTE = 'push-to-remote'
 CONFIG_COMMIT_MSG = 'commit_msg'
 
 def _get_current_branch():
@@ -54,12 +54,12 @@ def _do_update(golden_manager, config):
 
   branch = config[CONFIG_GOLDENS_BRANCH]
   src_dir = config[CONFIG_NEW_SRC_DIR]
-  auto_commit = config[CONFIG_AUTO_COMMIT]
+  push_to_remote = config[CONFIG_PUSH_TO_REMOTE]
   commit_msg = config[CONFIG_COMMIT_MSG]
   golden_manager.source_from(src_dir, commit_msg, branch,
                              updates=updates,
                              deletes=deletes,
-                             push_to_remote=auto_commit)
+                             push_to_remote=push_to_remote)
 
 def _get_deletes_updates(update_dir, golden_dir):
   ret_delete = []
@@ -95,7 +95,8 @@ def _interactive_mode(base_golden_dir):
     if code != 0:
       print('Failed to generate new goldens')
       exit(1)
-    config[CONFIG_NEW_SRC_DIR] = os.path.join(os.getcwd(), './out/renderdiff_tests/')
+    # Note that this matches RENDER_OUTPUT_DIR in preamble.sh
+    config[CONFIG_NEW_SRC_DIR] = os.path.join(os.getcwd(), './out/renderdiff/renders')
   else:
     def validator(src_dir):
       if not os.path.exists(src_dir):
@@ -136,16 +137,23 @@ def _interactive_mode(base_golden_dir):
     config[CONFIG_GOLDENS_DELETES] = []
     config[CONFIG_GOLDENS_UPDATES] = []
 
-  config[CONFIG_AUTO_COMMIT] = \
+  config[CONFIG_PUSH_TO_REMOTE] = \
       prompt_helper(f'Commit golden repo changes to remote?') == PROMPT_YES
   return config
 
 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')
+
+  # write-to-branch mode
   parser.add_argument('--source', help='Directory containing the new goldens')
   parser.add_argument('--commit-msg', help='Message for the commit to the golden repo')
-  parser.add_argument('--golden-repo-token', help='Access token for the golden repo')
+
+  # merge-to-main mode (used in postsubmit)
+  parser.add_argument('--merge-to-main', action="store_true", help='Merge to main the given branch')
+  parser.add_argument('--filament-tag', help='Tag to append to the commit message on merge')
 
   args, _ = parser.parse_known_args(sys.argv[1:])
   config = {}
@@ -155,17 +163,24 @@ if __name__ == "__main__":
       access_token=args.golden_repo_token
   )
   base_golden_dir = golden_manager.directory()
+
+  # This is the write-to-branch mode
   if args.branch and args.source and args.commit_msg:
     assert os.path.exists(args.source), f'{args.source} (--source) directory not found'
     deletes, updates = _get_deletes_updates(args.source, base_golden_dir)
     config = {
-        CONFIG_AUTO_COMMIT: True,
+        CONFIG_PUSH_TO_REMOTE: args.push_to_remote,
         CONFIG_GOLDENS_BRANCH: args.branch,
         CONFIG_NEW_SRC_DIR: args.source,
         CONFIG_GOLDENS_UPDATES: updates,
         CONFIG_GOLDENS_DELETES: deletes,
         CONFIG_COMMIT_MSG: args.commit_msg,
     }
+    _do_update(golden_manager, config)
+  # This is the merge-to-main mode
+  elif args.branch and args.merge_to_main and args.filament_tag:
+    golden_manager.merge_to_main(branch=args.branch, tag=args.filament_tag, push_to_remote=True)
+  # Else, we're in interactive mode of write-to-branch (for local execution).
   else:
     config = _interactive_mode(base_golden_dir)
-  _do_update(golden_manager, config)
+    _do_update(golden_manager, config)

test/renderdiff/test.sh

@@ -16,11 +16,22 @@
 
 source `dirname $0`/src/preamble.sh
 
-start_ && \
-    bash `dirname $0`/generate.sh && \
-    python3 ${RENDERDIFF_TEST_DIR}/src/golden_manager.py --output=${GOLDEN_OUTPUT_DIR} && \
+start_
+
+if [[ "$GITHUB_WORKFLOW" ]]; then
+    # The commit message would have been piped as stdin to this script
+    COMMIT_MSG=$(cat)
+    GOLDEN_BRANCH=$(echo "${COMMIT_MSG}" | python3 test/renderdiff/src/commit_msg.py)
+else
+    GOLDEN_BRANCH=$(git log -1 | python3 test/renderdiff/src/commit_msg.py)
+fi
+
+bash `dirname $0`/generate.sh && \
+    python3 ${RENDERDIFF_TEST_DIR}/src/golden_manager.py \
+            --branch=${GOLDEN_BRANCH} \
+            --output=${GOLDEN_OUTPUT_DIR} && \
     python3 ${RENDERDIFF_TEST_DIR}/src/compare.py \
             --src=${GOLDEN_OUTPUT_DIR} \
             --dest=${RENDER_OUTPUT_DIR} \
-            --out=${DIFF_OUTPUT_DIR} && \
-    end_
+            --out=${DIFF_OUTPUT_DIR}
+end_