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

[Tooling] Disable widgets script on L10n download #17682

Merged

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Dec 17, 2021

As mentioned in #17672 (comment)

⚠️ This PR targets the release branch, so that this fix would be applied before the next new_beta_release or finalize_release.

cc @jkmassel @mokagio You'll need to land this before running the next new_beta_release — otherwise the lane will likely crash (on not finding the strings files for widgets anymore)

Why?

Now that the Widgets & ShareExtension don't use their own *.lproj/Localizable.strings files (but instead reference the app bundle's one) — see #17630 & #17636 — this means we don't need to "redistribute" the strings downloaded from GlotPress from the main .strings to the ones in the Widget target's subfolders (as those don't exist anymore).

I am still working on porting the "download-from-glotpress-and-postprocess" part of the tooling to the release-toolkit and modernizing it by cleaning up the logic, but in the meantime, this PR just disables the call to extract-framework-translations.swift which was responsible for that "redistribution" of strings to Widgets, because it would otherwise fail and crash during new_beta_release and finalize_release when fetching the latest translations.

This is because the extract-framework-translations.swift script references the Base.lproj/Localizable.strings files from the Widget and ShareExtension subfolders (so that it knew which keys to copy over), but those files no longer exists since #17630, #17636 and #17672, so the script would not be happy with them missing.

Since all this logic will soon be replaced by new actions I'm working on in the release-toolkit, I only focused on disabling what would crash for now, while still keeping the rest of the current tooling around until the new stuff is ready.

…ng is still WIP

We are currently working on removing `update-translations.rb` + `fix-translations` + `extract-framework-translations.swift` scripts from the repo and migrating their logic into the release-toolkit, but that work is still in progress.

The role of the (misleadingly named) `extract-frameworks-translations.swift` script was to redistribute the keys from the `*.lproj/Localizable.strings` downloaded from GlotPress into the `.strings` files of the Widgets and ShareExtension targets, back when those targets used their own `.strings` file.

This is no longer the case since #17630 and #17636 landed – as Widgets and ShareExtension's code now reference the app's `.strings` file directly – which means we don't need to run the logic from `extract-frameworks-translations.swift` anymore, and in fact if we did, it would probably crash now that the `Base.lproj/Localizable.strings` files in the Widgets and ShareExtension subfolders (which that script references) don't exist anymore.
@peril-wordpress-mobile
Copy link

You can trigger an installable build for these changes by visiting CircleCI here.

@AliSoftware AliSoftware requested a review from a team December 17, 2021 17:07
@AliSoftware AliSoftware changed the base branch from develop to release/18.9 December 17, 2021 17:07
@AliSoftware AliSoftware self-assigned this Dec 17, 2021
@AliSoftware AliSoftware added the Tooling Build, Release, and Validation Tools label Dec 17, 2021
@AliSoftware AliSoftware added this to the 18.9 ❄️ milestone Dec 17, 2021
@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 17, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all this logic will soon be replaced by new actions I'm working on in the release-toolkit, I only focused on disabling what would crash for now, while still keeping the rest of the current tooling around until the new stuff is ready.

Makes sense, also considering this is a PR targeting a release branch, which we always try to keep as limited in scope as possible 👍

@mokagio
Copy link
Contributor

mokagio commented Dec 20, 2021

Update: When leaving my review, I didn't realize @AliSoftware would have been AFK today and till the new year.

I'm going to merge this, then, to avoid it getting stale and to cover our basis in regards to:

You'll need to land this before running the next new_beta_release — otherwise the lane will likely crash (on not finding the strings files for widgets anymore)

@mokagio mokagio merged commit ef12076 into release/18.9 Dec 20, 2021
@mokagio mokagio deleted the tooling/disable-widgets-script-on-l10n-download branch December 20, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tooling Build, Release, and Validation Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants