Skip to content
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

New Crowdin updates #9030

Merged
merged 66 commits into from
Jun 23, 2023
Merged

New Crowdin updates #9030

merged 66 commits into from
Jun 23, 2023

Conversation

xTVaser
Copy link
Sponsor Member

@xTVaser xTVaser commented Jun 22, 2023

No description provided.

@xTVaser
Copy link
Sponsor Member Author

xTVaser commented Jun 22, 2023

Regarding not running a bunch of builds everytime this happens, I weighed out the options that are available to do so, and I decided to just make these commits not run CI at all.

This feels fine as these PRs should only ever be updating the .ts files, and it's reasonably trust-worthy that they are in a valid format as they are being machine generated. If in the rare event they do cause a problem -- the release builds will catch it.

crowdin didn't clean them up since they were already there the first time it sync'd (before i changed the config file to use `locale`)
@Felipefpl
Copy link

Recent updates were added on crowdin, wouldn't be a better idea to wait till ppl had time to update their translations? I'm a translator myself and i finished my work but there are several ppl there still updating the translations. I hope we'll have at least 7 days to update the translations before 2.0.0 is released. 👍

@RedDevilus
Copy link
Contributor

RedDevilus commented Jun 22, 2023

Next release is still going to take quite awhile, translators have waited years at this point.

Though having waited this long has streamlined the process and most stuff are translatable from English tooltips and others.

I wouldnt worry about it. In the past translations were done the most in feature freeze which is not the period we are in yet.

Besides revisions will always happen or be needed. Probably going to take an audit to see if it needs more time to be more viable for real release and not just nightly as-is.

@stenzek
Copy link
Member

stenzek commented Jun 23, 2023

Recent updates were added on crowdin, wouldn't be a better idea to wait till ppl had time to update their translations?

We need to get the translation sources into the tree so that they can be hooked up to Qt, tested, and any fonts for ImGui rendering set (still need to figure that out for Linux..)

Edit: Probably should clarify, we're not releasing yet, and translation updates will get pulled periodically, so there's no issue with making changes later. This is just pulling what's there in, so we can make sure everything works.

@stenzek
Copy link
Member

stenzek commented Jun 23, 2023

Regarding not running a bunch of builds everytime this happens, I weighed out the options that are available to do so, and I decided to just make these commits not run CI at all.

Part of the issue here is the fact that the CI runs for every commit pushed to every branch. This is also an issue with master builds - it duplicates and runs both a PR-style workflow, as well as the release workflow.

I'd propose skipping such builds if the repository is pcsx2. If I remember correctly, part of the reason for having them run on commit push was so that the CI could run on forks, so this wouldn't break that functionality.

(also, an aside, we'd want to squash all these when merging.)

@xTVaser
Copy link
Sponsor Member Author

xTVaser commented Jun 23, 2023

Part of the issue here is the fact that the CI runs for every commit pushed to every branch. This is also an issue with master builds - it duplicates and runs both a PR-style workflow, as well as the release workflow.
I'd propose skipping such builds if the repository is pcsx2. If I remember correctly, part of the reason for having them run on commit push was so that the CI could run on forks, so this wouldn't break that functionality.

I'm aware of this issue, it's not so CI could run on forks, it's because some people wanted builds pushed to pcsx2 branches to run on CI without having to make a PR. An easy solution is to whitelist a pattern instead of just allowing all branches to build, but I have no idea what the current opinions are for this.

But yeah even if that was fixed it would atleast eliminate half the builds, but each commit's PR run would still trigger. It's unfortunate that Crowdin does a single commit per file.

@refractionpcsx2
Copy link
Member

refractionpcsx2 commented Jun 23, 2023

I'm aware of this issue, it's not so CI could run on forks, it's because some people wanted builds pushed to pcsx2 branches to run on CI without having to make a PR. An easily solution is to whitelist a pattern instead of just allowing all branches to build, but I have no idea what the current opinions are for this.

This has already been resolved :) Sten made it so it skips the branch builds, if the branch is the main repo, so forks still run

see #9038

@xTVaser
Copy link
Sponsor Member Author

xTVaser commented Jun 23, 2023

ah I see, if people are happy with that it's a very good change especially with how long the builds are.

@refractionpcsx2
Copy link
Member

It's been something that's annoyed me for a long time, since I do all my PR's on the main repo :D

@xTVaser xTVaser merged commit a46b3f2 into master Jun 23, 2023
12 checks passed
@xTVaser xTVaser deleted the l10n_master branch June 23, 2023 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants