-
Notifications
You must be signed in to change notification settings - Fork 850
Error when no new messages are downloaded from crowdin. #13986
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: develop
Are you sure you want to change the base?
Conversation
Build Artifacts
|
| i18n-download-translations: i18n-extract-frontend | ||
| touch kolibri/locale/.crowdin-download-marker | ||
| yarn exec crowdin download -- --branch ${CROWDIN_BRANCH} | ||
| @if [ -z "$$(find kolibri/locale/*/LC_MESSAGES -type f \( -name '*.po' -o -name '*.csv' \) -newer kolibri/locale/.crowdin-download-marker 2>/dev/null)" ]; then \ |
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.
If no files newer than the marker are found after running the command then we show the message ya? I mentally parsed out this bash and it looks right to me too.
Did you test out the idea with known newer files or otherwise force the situation you expect to trigger this error w/ test files or something to be sure (or see it work by triggering the error?)
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 personally didn't, but I did watch Claude Code do this exact test in front of me - I am happy to reproduce the experiment to validate though.
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.
Actually, the simplest way to test this was just to run this with and without the crowdin download commented out - with it commented out I got this output:
yarn run makemessages
yarn run v1.22.22
$ kolibri-i18n extract-messages --pluginFile ./build_tools/build_plugins.txt --namespace kolibri-common --searchPath ./packages/kolibri-common --namespace kolibri-core --searchPath ./packages/kolibri
Gathering relevant modules from ['kolibri.core', 'kolibri.plugins.*']
Writing webpack_json output to /tmp/20251116-1590230-1bij8nv.5g3m.json
INFO: Writing webpack_json output to /tmp/20251116-1590230-1bij8nv.5g3m.json
[WARN: root] Skipping /var/home/richard/github/kolibri/kolibri/plugins/coach/assets/src/views/common/LearnerExerciseReport.vue because $trs property was given an empty object
Done in 4.42s.
touch kolibri/locale/.crowdin-download-marker
❌ ERROR: No translation files were downloaded - Crowdin download may have failed silently
Check the output above for errors during the download process
make: *** [Makefile:238: i18n-download-translations] Error 1
With it still included, I got a 0 error code and plenty of messages!
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.
w00t! right on thanks for running it through to be sure :)
Summary
To workaround crowdin/crowdin-cli#965 add an explicit check that downloading from crowdin updates message files and error if not.
Adds a temporary marker file to then compare the message files to make sure they're newer than it.
References
Fixes issues seen in CI for message download
Reviewer guidance
I think this will help, but bit hard to test without reproducing the non-deterministic error :/