-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a GitHub workflow to run updpkgsums
in PRs
#62
Conversation
One part of the `open-pr` workflow is concerned with initializing a subset of Git for Windows' SDK that is able to run `pacman` and friends. We are about to introduce another workflow that needs this, so let's extract that code into its own composite GitHub Action. This commit is best viewed with `--color-moved --color-moved-ws=allow-indentation-change`. Signed-off-by: Johannes Schindelin <[email protected]>
.github/workflows/updpksums.yml
Outdated
update_script="$GITHUB_WORKSPACE/update-scripts/checksums/$PACKAGE_TO_UPGRADE" && | ||
if test -f "$update_script" | ||
then | ||
$update_script "$UPGRADE_TO_VERSION" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "$UPGRADE_TO_VERSION"
argument seems superfluous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! But the only checksums
script (the one for mingw-w64-git-lfs
) needs the version. So I added code for that:
diff --git a/.github/workflows/updpksums.yml b/.github/workflows/updpksums.yml
index a3215b1..3a83f35 100644
--- a/.github/workflows/updpksums.yml
+++ b/.github/workflows/updpksums.yml
@@ -137,7 +137,9 @@ jobs:
update_script="$GITHUB_WORKSPACE/update-scripts/checksums/$PACKAGE_TO_UPGRADE" &&
if test -f "$update_script"
then
- $update_script "$UPGRADE_TO_VERSION"
+ PACKAGE_VERSION="$(sed -n 's/^pkgver=//p' PKGBUILD)" &&
+ test -n "$PACKAGE_VERSION" &&
+ $update_script "$PACKAGE_VERSION"
else
updpkgsums
fi &&
.github/workflows/updpksums.yml
Outdated
fi && | ||
|
||
msg="$PACKAGES: update checksums" && | ||
git commit -sm "$msg" PKGBUILD && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're looping through 1 or more packages above, couldn't that result in multiple PKGBUILD files needing to be commited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right you are! I squashed this:
diff --git a/.github/workflows/updpksums.yml b/.github/workflows/updpksums.yml
index 3a83f35..340ae90 100644
--- a/.github/workflows/updpksums.yml
+++ b/.github/workflows/updpksums.yml
@@ -162,7 +162,7 @@ jobs:
fi &&
msg="$PACKAGES: update checksums" &&
- git commit -sm "$msg" PKGBUILD &&
+ git commit -asm "$msg" &&
echo "msg=$msg" >>$GITHUB_OUTPUT &&
echo "modified=true" >>$GITHUB_OUTPUT
- name: Determine target repository
.github/workflows/updpksums.yml
Outdated
git commit -sm "$msg" PKGBUILD && | ||
echo "msg=$msg" >>$GITHUB_OUTPUT && | ||
echo "modified=true" >>$GITHUB_OUTPUT | ||
- name: Determine target repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we know the repository already. We're determining some details for the push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I squashed this fix into the patch:
diff --git a/.github/workflows/updpksums.yml b/.github/workflows/updpksums.yml
index 340ae90..f0a41c9 100644
--- a/.github/workflows/updpksums.yml
+++ b/.github/workflows/updpksums.yml
@@ -165,7 +165,11 @@ jobs:
git commit -asm "$msg" &&
echo "msg=$msg" >>$GITHUB_OUTPUT &&
echo "modified=true" >>$GITHUB_OUTPUT
- - name: Determine target repository
+ - name: Determine repository to push to
+ # PRs frequently originate from forks; Unless contributors uncheck the box to
+ # allow maintainers to push to their PR branch, the GitForWindowsHelper GitHub
+ # App credentials are still good enough to push there, we just need to figure
+ # out where "there" is.
if: steps.update.outputs.modified == 'true'
id: determine-repository
uses: actions/github-script@v6
One of the most common problems in Git for Windows' PRs is that the checksums in the `PKGBUILD` files need to be updated. And the resolution is always the same: run `pkgpkgsums` and commit the result. That's so totally automatable, so let's do exactly that: automate it. Signed-off-by: Johannes Schindelin <[email protected]>
@rimrul thank you for the review! I believe that I have addressed all mentioned issues. |
Every once in a while, the Git for Windows project receives Pull Requests from new contributors, which is great!
It can be quite frustrating, then, if something goes awry due to the unintuitive and at times arcane paradigms, e.g. Pacman's checksums that are ignored during an
sdk build
(because runningupdpkgsums
is slow and gets in the way of developing exciting features).As suggested in git-for-windows/gfw-helper-github-app#28, we should have automation for this. Here is the
git-for-windows-automation
part, and I already used a development version of this GitHub workflow (triggered onpush
becauseworkflow_dispatch
can only be triggered if the default branch has a workflow of the same name) to update git-for-windows/MINGW-packages#104, partially to prove that this thing works (it was not totally clear to me whether the GitHub App would be permitted to push the PR branch).