autoformat: change from hg fix to tip commit/amend strategy (Bug 1785004) by cgsheeh · Pull Request #212 · mozilla-conduit/lando-api

cgsheeh · 2022-08-16T01:46:01Z

Remove the hg fix based autoformatting and replace with
running raw mach lint on the tip commit, creating a tip
autoformat commit for stacks of changes and amending the
commit for landings of a single changeset. Add a call to
mach bootstrap in the repo clone subsystem when autoformat
is enabled for a given repo, to enabled downloading required
packages. Since we now run mach from within the landing repo,
stop running hg purge with --all since that will remove
the existing objdir required for linting virtualenvs. Add the
MOZBUILD_STATE_PATH environment variable to the Dockerfile
to avoid a prompt during mach bootstrap.

zzzeid

Couple of small comments.

zzzeid · 2022-08-16T16:12:19Z

landoapi/hg.py


-            fix_hg_command += ["--config", f"fix.{key}={value}"]
+        # Run linters.
+        subprocess.run([mach_path, "lint", "-r", "stack()"], check=True)


Wondering if it would be useful to store the command and its output, and put that in the commit message for verbosity?

That's not a bad idea - we should definitely at least log the command output.

zzzeid · 2022-08-16T16:12:37Z

landoapi/hg.py

+        except hglib.error.CommandError as exc:
+            if exc.out.strip() == b"nothing changed":
+                # If nothing changed after formatting we can just return.
+                return None


Suggested change

return None

return

zzzeid · 2022-08-16T16:14:22Z

landoapi/hg.py


-        logger.info(f"revisions were reformatted: {', '.join(post_formatting_hashes)}")
+            if formatting_changeset is None:
+                return None


Suggested change

return None

return

…85004) Remove the `hg fix` based autoformatting and replace with running raw `mach lint` on the tip commit, creating a tip autoformat commit for stacks of changes and amending the commit for landings of a single changeset. Add a call to `mach bootstrap` in the repo clone subsystem when autoformat is enabled for a given repo, to enabled downloading required packages. Since we now run `mach` from within the landing repo, stop running `hg purge` with `--all` since that will remove the existing objdir required for linting virtualenvs. Add the `MOZBUILD_STATE_PATH` environment variable to the Dockerfile to avoid a prompt during `mach bootstrap`.

zzzeid

Mostly small comments or minor changes.

zzzeid · 2022-11-22T17:57:53Z

landoapi/hg.py

        except hglib.error.CommandError:
            pass
        try:
-            self.run_hg(["purge", "--all"])


Are there other unforeseen circumstances of removing --all here? (e.g., why did we have it here in the first place?)

We wanted the working copy to be clean after calling clean_repo, so I assume it was added since we didn't have a need for any VCS ignored files to be retained after cleaning. However we now need to keep at least the objdir and parts of third_party in central, so just running purge should be good enough for our purposes.

zzzeid · 2022-11-22T18:05:01Z

landoapi/hg.py

+        except hglib.error.CommandError as exc:
+            if exc.out.strip() == b"nothing changed":
+                # If nothing changed after formatting we can just return.
+                return None


nit

Suggested change

return None

return

Meh, personally I prefer to be explicit that None is going to be returned from a function, especially when the type is an Optional. I usually only ever do a generic return when the function itself would never return something. I'll change it in this case. 👍

zzzeid · 2022-11-22T18:06:29Z

landoapi/hg.py

-        # Exit if no revisions were reformatted.
-        pre_formatting_hashes = check_fix_output_for_replacements(fix_output)
-        if not pre_formatting_hashes:
+    def format_stack_amend(self) -> Optional[List[str]]:


Not very certain why we should treat 1-revision stacks any differently than multi-revision stacks.

Since we know formatting changes made to 1-revision stacks all apply to the revision, we can just amend them in. This avoids creating any autoformatting commits for the most common type of landings.

landoapi/hg.py

zzzeid · 2022-11-22T18:09:50Z

landoapi/repos.py

+            if repo.autoformat_enabled:
+                r.run_mach_bootstrap()


This my cause some problems if there are changes to mach bootstrap that need to be applied after the instance is started (since the ready method is only triggered once per restart).

For now I think we can run this at repo setup time - we may need to add a mechanism to periodically re-call mach bootstrap to keep toolchains up to date. I'd like to land these changes first before optimizing too much. :)

zzzeid · 2022-11-22T18:10:55Z

landoapi/landing_worker.py


-            # Run `hg fix` configured formatters if enabled
+            # Run automated code formatters if enabled.
            if repo.autoformat_enabled:


As discussed offline, this may be a good candidate to move to the Revision Worker (based on PR #224) in the future, and implement a way for the revision worker to store updated stacks for the landing worker to use.

…ase removed from template)

zzzeid

lgtm!

cgsheeh requested a review from zzzeid August 16, 2022 01:46

zzzeid requested changes Aug 16, 2022

View reviewed changes

cgsheeh force-pushed the 1785004 branch from e799b14 to e898467 Compare November 18, 2022 18:57

cgsheeh requested a review from zzzeid November 18, 2022 18:57

cgsheeh changed the title ~~autoformat: change from hg fix strategy to tip commit strategy (Bug 1785004)~~ autoformat: change from hg fix to tip commit/amend strategy (Bug 1785004) Nov 21, 2022

cgsheeh added 3 commits November 21, 2022 17:14

update comment to reflect amend

72af621

clarify comment about mozbuild state path in Dockerfile

6d4d73d

remove unnecessary comment about running linters

46e9e86

zzzeid requested changes Nov 22, 2022

View reviewed changes

cgsheeh added 3 commits November 22, 2022 14:17

move formatting and commit/amending into separate try/except blocks

f9cb224

return None -> return

29b9409

explicitly test that ignore-this-changeset is in commit message (in c…

35c0d78

…ase removed from template)

cgsheeh requested a review from zzzeid November 22, 2022 19:33

zzzeid approved these changes Nov 22, 2022

View reviewed changes

Merge branch 'main' into 1785004

0f37153

cgsheeh merged commit 736c94d into mozilla-conduit:main Nov 22, 2022

Conversation

cgsheeh commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zzzeid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zzzeid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zzzeid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cgsheeh commented Aug 16, 2022 •

edited

Loading