Skip to content

Commit 06a2e5d

Browse files
qinsoonqinsoon
authored
Allow auto merge binding PRs (#988)
This PR allows auto merging related binding PRs once the mmtk-core PR is merged. It also adds some docs for MMTk team about merging process. The PR closes #205.
1 parent 29d482c commit 06a2e5d

File tree

7 files changed

+317
-15
lines changed

7 files changed

+317
-15
lines changed

‎.github/scripts/replace-mmtk-dep.py

Lines changed: 55 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,35 +2,77 @@
22

33
import argparse
44
import os.path
5+
import sys
56
import tomlkit
67

78
parser = argparse.ArgumentParser(
89
description='Replace the mmtk-core dependency of a given VM binding',
910
)
1011

1112
parser.add_argument('toml_path', help='Path to Cargo.toml')
12-
parser.add_argument('mmtk_core_path', help='Path to the mmtk_core repo')
13+
# The following arguments are exclusive. Use either. If both are supplied, we use the local path.
14+
# 1. Point to a local path
15+
parser.add_argument('--mmtk-core-path', help='Path to the mmtk-core repo.')
16+
# 2. Point to a remote repo
17+
parser.add_argument('--mmtk-core-git', help='URL to the mmtk-core repo.')
18+
parser.add_argument('--mmtk-core-rev', help='Revision to use')
1319

1420
args = parser.parse_args()
1521

22+
# Check what we should do.
23+
if args.mmtk_core_path is not None:
24+
how = "point_to_local"
25+
elif args.mmtk_core_git is not None and args.mmtk_core_rev is not None:
26+
how = "point_to_repo"
27+
else:
28+
print("No path or git/rev is supplied. We cannot update the toml")
29+
sys.exit(1)
30+
1631
print("Reading TOML from '{}'".format(args.toml_path))
1732
with open(args.toml_path, "rt") as f:
1833
toml_data = tomlkit.load(f)
1934

35+
if "mmtk" not in toml_data["dependencies"]:
36+
print("Cannot find the mmtk dependency in {}".format(args.toml_path))
37+
sys.exit(1)
38+
39+
# The mmtk dependency could be an inlined table for some bindings:
40+
# [dependencies]
41+
# mmtk = { git = "...", rev = "..." }
42+
# But it could be a subtable for other bindings:
43+
# [dependencies.mmtk]
44+
# git = "..."
45+
# rev = "..."
2046
mmtk_node = toml_data["dependencies"]["mmtk"]
2147

22-
# These keys may specify the locations of the dependency. Remove them.
23-
for key in ["git", "branch", "version", "registry"]:
24-
if key in mmtk_node:
25-
print("Deleting dependencies.mmtk.{}".format(key))
26-
del mmtk_node[key]
27-
else:
28-
print("Key dependencies.mmtk.{} does not exist. Ignored.".format(key))
29-
30-
# Use mmtk-core from the specified local directory.
31-
mmtk_repo_path = os.path.realpath(args.mmtk_core_path)
32-
print("Setting dependencies.mmtk.path to {}".format(mmtk_repo_path))
33-
mmtk_node["path"] = mmtk_repo_path
48+
def remove_keys(item, keys):
49+
for key in keys:
50+
if key in item:
51+
print("Deleting dependencies.mmtk.{}".format(key))
52+
del item[key]
53+
else:
54+
print("Key dependencies.mmtk.{} does not exist. Ignored.".format(key))
55+
56+
57+
if how == "point_to_local":
58+
# We are going to update the dependency to a local path. First remove anything we know that would set a version.
59+
# Deleting all the keys will mess up the formatting. But it doesn't matter, as the file with
60+
# a dependency to the local path is just temporary.
61+
remove_keys(mmtk_node, ["git", "branch", "registry", "rev", "tag", "path", "version"])
62+
63+
# Use mmtk-core from the specified local directory.
64+
mmtk_repo_path = os.path.realpath(args.mmtk_core_path)
65+
mmtk_node["path"] = mmtk_repo_path
66+
print("Setting dependencies.mmtk.path to {}".format(mmtk_repo_path))
67+
elif how == "point_to_repo":
68+
# We assume the file already includes `git` and `rev`. We just update them. This will preserve the
69+
# original formatting and comments.
70+
71+
# Update git/rev
72+
mmtk_node["git"] = args.mmtk_core_git
73+
mmtk_node["rev"] = args.mmtk_core_rev
74+
print("Setting dependencies.mmtk to git={},rev={}".format(args.mmtk_core_git, args.mmtk_core_rev))
75+
3476

3577
print("Writing TOML to '{}'".format(args.toml_path))
3678
with open(args.toml_path, "wt") as f:

‎.github/workflows/auto-merge-inner.yml

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
name: Merge Binding PR
2+
3+
on:
4+
workflow_call:
5+
inputs:
6+
# The repository from which the binding PR is submitted, such as qinsoon/mmtk-openjdk
7+
repo:
8+
required: true
9+
type: string
10+
# The upstream repository where the binding PR is opened, such as mmtk/mmtk-openjdk
11+
base_repo:
12+
required: true
13+
type: string
14+
# The branch name for the PR
15+
ref:
16+
required: true
17+
type: string
18+
# The core commit hash that the binding should be using.
19+
core_commit:
20+
required: true
21+
type: string
22+
# the command line to update lock file once we update Cargo.toml
23+
update_lockfile:
24+
required: true
25+
type: string
26+
27+
env:
28+
MMTK_CORE_WORK_DIR: mmtk-core
29+
BINDING_WORK_DIR: mmtk-binding-repo
30+
31+
jobs:
32+
process-pr:
33+
runs-on: ubuntu-latest
34+
steps:
35+
- name: Check input conditions
36+
id: check-input
37+
run: |
38+
if [[ "${{ inputs.repo }}" == ${{ inputs.base_repo }} ]] && [[ "${{ inputs.ref }}" == "master" ]]; then
39+
echo "Conditions not met"
40+
echo "::set-output name=skip::true"
41+
else
42+
echo "::set-output name=skip::false"
43+
fi
44+
shell: bash
45+
46+
- name: Checkout MMTk Core
47+
uses: actions/checkout@v3
48+
if: steps.check-input.outputs.skip == 'false'
49+
with:
50+
path: ${{ env.MMTK_CORE_WORK_DIR }}
51+
- name: Checkout repository
52+
if: steps.check-input.outputs.skip == 'false'
53+
uses: actions/checkout@v3
54+
with:
55+
repository: ${{ inputs.repo }}
56+
path: ${{ env.BINDING_WORK_DIR }}
57+
ref: ${{ inputs.ref }}
58+
# Check out with CI_ACCESS_TOKEN so we can push to it.
59+
token: ${{ secrets.CI_ACCESS_TOKEN }}
60+
61+
- name: Get PR number
62+
if: steps.check-input.outputs.skip == 'false'
63+
id: get-pr
64+
run: |
65+
PR_NUMBER=$(gh pr list --head ${{ inputs.ref }} --repo ${{ inputs.base_repo }} --json number --jq '.[0].number')
66+
echo "::set-output name=pr_number::$PR_NUMBER"
67+
shell: bash
68+
env:
69+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
70+
71+
- name: Update mmtk dependency
72+
if: steps.check-input.outputs.skip == 'false'
73+
working-directory: ${{ env.MMTK_CORE_WORK_DIR }}
74+
run: |
75+
export MANIFEST_PATH=../${{ env.BINDING_WORK_DIR }}/mmtk/Cargo.toml
76+
./.github/scripts/ci-replace-mmtk-dep.sh $MANIFEST_PATH --mmtk-core-git https://github.com/mmtk/mmtk-core.git --mmtk-core-rev ${{ inputs.core_commit }}
77+
${{ inputs.update_lockfile }} --manifest-path $MANIFEST_PATH
78+
79+
- name: Push a new commit
80+
if: steps.check-input.outputs.skip == 'false'
81+
working-directory: ${{ env.BINDING_WORK_DIR }}
82+
run: |
83+
git config user.name "mmtkgc-bot"
84+
git config user.email "mmtkgc.bot@gmail.com"
85+
git add mmtk/Cargo.toml
86+
git add mmtk/Cargo.lock
87+
git commit -m "Update mmtk-core to ${{ inputs.core_commit }}"
88+
git push
89+
90+
- name: Enable auto-merge for the PR
91+
if: steps.check-input.outputs.skip == 'false'
92+
run: |
93+
gh pr merge ${{ steps.get-pr.outputs.pr_number }} --auto --repo ${{ inputs.base_repo }} --squash
94+
env:
95+
GITHUB_TOKEN: ${{ secrets.CI_ACCESS_TOKEN }}

‎.github/workflows/auto-merge.yml

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
name: Auto Merge Binding PRs
2+
3+
# Only trigger on push to master. The workflow needs to use repo secrets.
4+
# Triggering on master can make sure we have secrets.
5+
on:
6+
push:
7+
branches:
8+
- master
9+
10+
jobs:
11+
# Figure out the PR that is merged.
12+
get-merged-pr:
13+
runs-on: ubuntu-latest
14+
outputs:
15+
commit: ${{ steps.get-commit-hash.outputs.commit }}
16+
pr: ${{ steps.get-pr.outputs.pr }}
17+
steps:
18+
- id: get-commit-hash
19+
run: echo "commit=${GITHUB_SHA}" >> "$GITHUB_OUTPUT"
20+
- id: get-pr
21+
run: |
22+
PR_NUMBER=$(gh pr list --search "${GITHUB_SHA}" --state merged --repo mmtk/mmtk-core --json number --jq '.[0].number')
23+
echo "pr=$PR_NUMBER" >> "$GITHUB_OUTPUT"
24+
env:
25+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
26+
27+
# Figure out binding PRs.
28+
binding-refs:
29+
uses: ./.github/workflows/pr-binding-refs.yml
30+
needs: get-merged-pr
31+
with:
32+
pull_request: ${{ needs.get-merged-pr.outputs.pr }}
33+
34+
check-merge-openjdk-pr:
35+
uses: ./.github/workflows/auto-merge-inner.yml
36+
needs: [get-merged-pr, binding-refs]
37+
with:
38+
repo: ${{ needs.binding-refs.outputs.openjdk_binding_repo }}
39+
base_repo: mmtk/mmtk-openjdk
40+
ref: ${{ needs.binding-refs.outputs.openjdk_binding_ref }}
41+
core_commit: ${{ needs.get-merged-pr.outputs.commit }}
42+
update_lockfile: cargo build
43+
secrets: inherit
44+
45+
check-merge-jikesrvm-pr:
46+
uses: ./.github/workflows/auto-merge-inner.yml
47+
needs: [get-merged-pr, binding-refs]
48+
with:
49+
repo: ${{ needs.binding-refs.outputs.jikesrvm_binding_repo }}
50+
base_repo: mmtk/mmtk-jikesrvm
51+
ref: ${{ needs.binding-refs.outputs.jikesrvm_binding_ref }}
52+
core_commit: ${{ needs.get-merged-pr.outputs.commit }}
53+
update_lockfile: cargo build --features nogc --target i686-unknown-linux-gnu
54+
secrets: inherit
55+
56+
check-merge-v8-pr:
57+
uses: ./.github/workflows/auto-merge-inner.yml
58+
needs: [get-merged-pr, binding-refs]
59+
with:
60+
repo: ${{ needs.binding-refs.outputs.v8_binding_repo }}
61+
base_repo: mmtk/mmtk-v8
62+
ref: ${{ needs.binding-refs.outputs.v8_binding_ref }}
63+
core_commit: ${{ needs.get-merged-pr.outputs.commit }}
64+
update_lockfile: cargo build --features nogc
65+
secrets: inherit
66+
67+
check-merge-julia-pr:
68+
uses: ./.github/workflows/auto-merge-inner.yml
69+
needs: [get-merged-pr, binding-refs]
70+
with:
71+
repo: ${{ needs.binding-refs.outputs.julia_binding_repo }}
72+
base_repo: mmtk/mmtk-julia
73+
ref: ${{ needs.binding-refs.outputs.julia_binding_ref }}
74+
core_commit: ${{ needs.get-merged-pr.outputs.commit }}
75+
update_lockfile: cargo build --features immix
76+
secrets: inherit
77+
78+
check-merge-ruby-pr:
79+
uses: ./.github/workflows/auto-merge-inner.yml
80+
needs: [get-merged-pr, binding-refs]
81+
with:
82+
repo: ${{ needs.binding-refs.outputs.ruby_binding_repo }}
83+
base_repo: mmtk/mmtk-ruby
84+
ref: ${{ needs.binding-refs.outputs.ruby_binding_ref }}
85+
core_commit: ${{ needs.get-merged-pr.outputs.commit }}
86+
update_lockfile: cargo build
87+
secrets: inherit

‎.github/workflows/merge-check.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ jobs:
2020
# - this action
2121
# - pre code review checks for stable Rust (we allow them to fail)
2222
# - binding tests (it may take long to run)
23-
ignoreActions: "ready-to-merge,check-public-api-changes,pre-code-review-checks/x86_64-unknown-linux-gnu/stable,pre-code-review-checks/i686-unknown-linux-gnu/stable,pre-code-review-checks/x86_64-apple-darwin/stable,v8-binding-test,openjdk-binding-test,jikesrvm-binding-test,julia-binding-test,ruby-binding-test"
23+
ignoreActions: "ready-to-merge,check-public-api-changes,pre-code-review-checks/x86_64-unknown-linux-gnu/stable,pre-code-review-checks/i686-unknown-linux-gnu/stable,pre-code-review-checks/x86_64-apple-darwin/stable,v8-binding-test,openjdk-binding-test,jikesrvm-binding-test,julia-binding-test,ruby-binding-test (release),ruby-binding-test (debug)"
2424
# This action uses API. We have a quota of 1000 per hour.
2525
checkInterval: 600
2626
env:

‎.github/workflows/post-review-ci.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ jobs:
200200
ref: mmtk
201201

202202
- name: Override mmtk-core dependency for binding
203-
run: ./.github/scripts/ci-replace-mmtk-dep.sh ../mmtk-ruby/mmtk/Cargo.toml .
203+
run: ./.github/scripts/ci-replace-mmtk-dep.sh ../mmtk-ruby/mmtk/Cargo.toml --mmtk-core-path .
204204
working-directory: mmtk-core
205205

206206
- name: Setup environment

‎docs/team/README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
This is the folder for public docs for the MMTk team. The MMTk team refers to the members in the
2+
MMTk GitHub organization.

‎docs/team/pull_request.md

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
# Pull Requests
2+
3+
## API Breaking Changes
4+
5+
If a PR includes API breaking changes, it is the responsibility of the MMTk team to make sure
6+
the officially supported bindings are updated to accomodate the changes.
7+
A team member may open a PR in the bindings him/herself if the required
8+
changes are trivial enough. Otherwise he/she may ask the team member that is responsible for a specific
9+
binding to request an update. The corresponding binding PRs need to be ready to merge before the mmtk-core PR
10+
can be merged.
11+
12+
### Testing PRs with API breaking changes
13+
14+
If an MMTk core PR should be tested with other bindings PRs, one can specify the binding branch that
15+
should be tested with by adding a comment like below (see https://github.com/mmtk/mmtk-core/blob/master/.github/workflows/pr-binding-refs.yml).
16+
If there are multiple comments that match, the first one is effective. If the info is missing for
17+
a binding, the default repo (`mmtk/mmtk-X`) and branch (`master`) will be used instead.
18+
```
19+
binding-refs
20+
OPENJDK_BINDING_REPO=xx/xx
21+
OPENJDK_BINDING_REF=xxxxxx
22+
JIKESRVM_BINDING_REPO=xx/xx
23+
JIKESRVM_BINDING_REF=xxxxxx
24+
V8_BINDING_REPO=xx/xx
25+
V8_BINDING_REF=xxxxxx
26+
JULIA_BINDING_REPO=xx/xx
27+
JULIA_BINDING_REF=xxxxxx
28+
RUBY_BINDING_REPO=xx/xx
29+
RUBY_BINDING_REF=xxxxxx
30+
```
31+
32+
### Merging a PR with API breaking changes
33+
34+
If an MMTk core PR includes API breaking changes, the corresponding binding PRs depends on an mmtk-core commit in the PR branch. As we
35+
use squashing merging, the commit in the PR branch will disappear once the mmtk-core PR is merged. When the mmtk-core PR is merged,
36+
we will have a new commit in `mmtk-core` master. We will need to fix the mmtk dependency in the binding PRs to point to the new commit,
37+
and then merge the binding PRs.
38+
39+
#### Auto merging process
40+
41+
This process should be done automatically by [`auto-merge.yml`](https://github.com/mmtk/mmtk-core/blob/master/.github/workflows/auto-merge.yml)
42+
when an mmtk-core PR is merged and the `binding-refs` comment is present.
43+
44+
1. Make sure there is no other PR in this merging process. If so, resolve those first.
45+
1. Make sure all the PRs (the mmtk-core PR, the binding PRs, and the associated PRs in the VM repo if any) are ready to merge.
46+
1. For each binding PR that we need to merge:
47+
1. If the binding PR has an assocate PR in the VM repo, merge the VM PR first. Once it is merged, we will have a commit hash (we refer to it as `{vm_commit}`).
48+
1. Update `mmtk/Cargo.toml` in the binding:
49+
* Find the section `[package.metadata.{binding-name}]`.
50+
* Update the field `{binding-name}_repo` if necessary. It should point to our VM fork, such as `https://github.com/mmtk/{binding-name}.git`.
51+
* Update the field `{binding-name}_version`. It should point to the new commit hash `{vm_commit}`.
52+
* Commit the change.
53+
1. Merge the mmtk-core PR.
54+
1. When a new commit is pushed to `master`, `auto-merge.yml` will be triggered.
55+
1. The binding PRs should be updated and auto merge will be eanbled for the PR. Keep an eye until the PRs are all merged. Resolve any
56+
issue that prevents the PR from being auto merged (e.g. flaky tests).
57+
58+
#### Manual merging process
59+
60+
If `auto-merge.yml` failed for any reason, or if we have to manually merge binding PRs, this is the process to follow:
61+
62+
1. Follow Step 1-4 in the auto merging process.
63+
1. When a new commit is pushed to `master`, we record the commit hash (as `{mmtk_core_commit}`).
64+
1. For each binding PR that we need to merge:
65+
1. Update `mmtk/Cargo.toml` in the binding:
66+
* Find the `mmtk` dependency under `[dependencies]`.
67+
* Update the field `git` if necessary. It should point to our mmtk-core repo, `https://github.com/mmtk/mmtk-core.git`.
68+
* Update the field `rev`. It should point to the new mmtk-core commit hash `{mmtk_core_commit}`.
69+
* Update `mmtk/Cargo.lock` by building the Rust project again. If the binding needs to choose a GC plan by feature, use
70+
any supported plan. So this step is slightly different for different bindings:
71+
* OpenJDK, Ruby: `cargo build`
72+
* JikesRVM: `cargo build --features nogc --target i686-unknown-linux-gnu`
73+
* V8: `cargo build --features nogc`
74+
* Julia: `cargo build --features immix`
75+
* Check in both `mmtk/Cargo.toml` and `mmtk/Cargo.lock`, and commit.
76+
2. Merge the PR once it can be merged.

0 commit comments

Comments
 (0)