-
Notifications
You must be signed in to change notification settings - Fork 258
[CI] Ensure messages.pot stays updated #808
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
base: dev
Are you sure you want to change the base?
[CI] Ensure messages.pot stays updated #808
Conversation
|
LGTM, ACK and Tested locally and in my forked repo with some test PRs. |
l10n/extract_messages.py
Outdated
|
|
||
| def main(): | ||
| if not POT.exists(): | ||
| print(f"{POT} does not exist. Make sure that your local repo has fetched the `seedsigner-translations` submodule.") |
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.
Separate TODO item (not part of this PR) but I don't see any basic instructions to make sure the submodules are initialized.
Should this instruction include --recursive?
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. I remember being confused about this when I first started contributing as well. We should definitely add instructions for cloning with submodules and/or initializing them if the repo was cloned without submodules.
I’m also not completely sure, but I think the newer flag is --recurse-submodules rather than --recursive?
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 sure you're right. I was just going by memory (ALSO why I need everything documented; my memory is terrible!).
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.
Also, I see there’s some relevant documentation on this: l10n/README.md that covers initializing submodules. It might be worth linking to this or adding a CONTRIBUTING.md with general instructions, possibly reusing some of the messages you shared in the Telegram group earlier this year during Summer of Bitcoin. This would make it easier for new contributors.
fc44a7e to
2bc71e6
Compare
2bc71e6 to
4260a24
Compare
4260a24 to
7d0f9d3
Compare
7d0f9d3 to
01c0ddc
Compare
|
After some offline discussion, we have a slightly different view of how we'd like to pursue this. Your PR would effectively says that an up to date messages.pot file must be part of any PR that touches text. But as other PRs are merged, any open PRs would then need to ensure that there are no merge conflicts with the just-updated messages.pot. Since our PRs tend to stay open for quite a while, that could create multiple rounds of rebasing and regenerating the messages.pot. That feels like an unnecessary additional maintenance task for all PR authors. That being said, there IS value in surfacing any expected changes in the messages.pot due to a given PR. So we were wondering if instead we could just do a Github Action that would act somewhat like the screenshot diff generator in the translations repo: Generate the new messages.pot and then compare that against a version generated from the latest The screenshot diff generator only went as far as outputting an html summary of the screenshot diffs, but you still have to dig a bit into the workflow results to get that report in the downloadable CI artifacts. Ideally this messages.pot diff Action would write a comment in the PR and report the results there. That's beyond my Github Actions know-how, though. And then on each new commit in the PR, the Action should be able to update its comment in place with the latest diff. For example, bitcoin core has some kind of automation that dynamically updates the list of ACKs and NACKs in the first comment in a PR. But note that with the approach described here, we are explicitly avoiding making the messages.pot a requirement to include in the PR. In fact, we'd like to move towards another automation that will rebuild messages.pot on each merge. Though now that I think of it, the only way for that to work would be for the Action to open a new PR with the resulting change. If you're onboard with this approach, I'd suggest splitting it into two PRs:
This would mean that we would abandon the pre-commit approach. But I think a lot of that python script can be adapted for the Step 1 job. |
|
That makes sense. I agree that requiring every PR to stay fully up to date with I still think there is value in keeping a pre-commit hook for developers. It removes the manual step of remembering to update So, aligning with your suggestion, I would propose the following:
For this specific PR, that would mean keeping the entire pre-commit setup, but removing the current GitHub workflow and replacing it with the above two workflows. Let me know what you think, and I can start implementing this. I am not fully sure yet what permissions the GitHub Actions bot will need, but it should be manageable. |
Description
This PR introduces a new CI job to ensure that the
messages.potfile is always up to date. It also adds apre-commithook that automatically runs theextract_messagesstep before each commit.Pre-commit hook
The hook runs a Python script that does the following:
python3 setup.py extract_messages.messages.potdiffers from the previous version, ignoring changes to the metadata fieldsPOT-Creation-DateandGenerated-By.Github Action
Similarly, the GitHub workflow runs the
pre-commithook. If it results in any changes, they will appear as a diff in the logs, and the action will fail. This makes it clear that the PR must be updated to include the corresponding changes tomessages.pot.Other changes
setuptoolsas a dependency inrequirements-l10n.txt.messages.potsince thedevbranch was not up to date.pre-commithook.I also considered adding
pre-commititself torequirements-l10n.txt, but decided against it. It’s not strictly an l10n dependency. We may want to add arequirements-dev.txtinstead, or even better, migrate fully topyproject.tomland/or a tool like uv.This pull request is categorized as a:
Checklist
pytestand made sure all unit tests pass before submitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: