-
Notifications
You must be signed in to change notification settings - Fork 80
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 workflow to update GHC bindists #1944
Conversation
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.
Thank you. A few questions, and I think this could be done easier via JSON. That said, AFAIU this re-uses the previous code from the manual update script. So, it's not much new complexity. So, I'm happy to merge this and switch to the JSON route in a follow-up.
.github/workflows/update-ghc.yaml
Outdated
# TODO: needs a token so that workflows for the PR get triggered | ||
#token: "${{ secrets.PR_TOKEN }}" |
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 think you have the required permissions to set such a token, or do you need help on that?
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 just wanted to check how other projects have set this up, maybe I could re-use a token -- do we have a bot user maybe?
Otherwise, I'll just create a new PAT and add it as a secret.
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'm not aware of a globally configured token or a bot user.
Otherwise, I'll just create a new PAT and add it as a secret.
That sounds good to me. I think you can use environment to limit which actions it's exposed to if that's a concern.
- '9.2' | ||
- '9.4' | ||
- '9.6' |
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.
Just to confirm my understanding. Why do we loop over major-minor versions via a matrix instead of doing that within the update script with a single action run? Is it so that if multiple updates fall onto the same run we still get separate PRs per major-minor 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.
Yes, the reason was that I wanted to have separate PRs for each stable GHC major.minor version.
if len(sys.argv) != 2: | ||
sys.exit(f"usage: {SCRIPT_PATH.name} GHC_MAJOR_MINOR") | ||
|
||
bindist = import_path(str(SCRIPT_PATH.parent.parent.joinpath('haskell/private/ghc_bindist_generated.bzl'))) |
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.
An easier route could be to replace haskell/private/ghc_bindist_generated.bzl
by a JSON file. The data is only loaded into the repository rule in haskell/ghc_bindist.bzl
, so a combination of repository_ctx.read
and json.decode
could achieve the same thing. I think the data is only stored in a .bzl
file for historic reasons (older versions of Bazel didn't expose JSON parsing in the Starlark API).
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 call. I'll do this in a follow-up PR.
This workflow is scheduled weekly and fetches the latest GHC release for each of the current stable versions. It adds the version to the gen_ghc_bindist.py file and runs this script to update the ghc_bindist_generated.bzl file.
f79eb3c
to
795968b
Compare
The workflow is scheduled once a week, and can be triggered manually.
Currently, this does not (yet) try to update the GHC version that is used for tests.
And the created PR do not trigger CI; we would need to use a different token for that.
cf. #1878