Skip to content

Run genstrings in CI to validate LocalizedString usages #19553

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Run genstrings in CI to validate LocalizedString usages #19553

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b7b071
Add `skip_commit` option to `generate_strings_file_for_glotpress` lane
mokagio Nov 1, 2022
66d8818
Add CI step to run `.strings` generation
mokagio Nov 1, 2022
448d398
Annotate build with failed `genestrings` messages
mokagio Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .buildkite/commands/lint-localized-strings-format.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/bash -eu

echo "--- :rubygems: Setting up Gems"
install_gems

echo "--- :cocoapods: Setting up Pods"
install_cocoapods
Copy link
Contributor Author

@mokagio mokagio Nov 2, 2022

Choose a reason for hiding this comment

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

This line made me go down a rabbit hole thinking about whether we can optimize how we generate the pods cache further #19551.


echo "--- :writing_hand: Copy Files"
mkdir -pv ~/.configure/wordpress-ios/secrets
cp -v fastlane/env/project.env-example ~/.configure/wordpress-ios/secrets/project.env

echo "--- Lint Localized Strings Format"
bundle exec fastlane generate_strings_file_for_glotpress skip_commit:true
Copy link
Contributor Author

@mokagio mokagio Nov 2, 2022

Choose a reason for hiding this comment

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

I'm debating whether to put in the time to somehow capture the genstrings error, if any, and to annotate the build with it, vs banking on this to be a low occurrence kind of failure and accepting a less polished UX.

Copy link
Contributor

@AliSoftware AliSoftware Nov 2, 2022

Choose a reason for hiding this comment

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

I'm not sure how easy it would be to capture the error from genstrings here anyway 🤔

Maybe one easy way would be to store the logs of bundle exec fastlane command (e.g. | tee logs.txt) then grep for genstrings: error: lines in it?

bundle exec fastlane generate_strings_file_for_glotpress skip_commit:true | tee genstrings-logs.txt
if [[ $? -ne 0 ]]; then
  # Previous step errored, extract the errors in an easy-to-find section
  echo "--- Report genstrings errors"
  echo "Found errors when trying to run genstrings to generate the `.strings` files from `*LocalizedStrings` calls."
  echo "See below for a summary (or previous section for more details)."
  grep 'genstrings: ' 'genstrings-logs.txt'
  grep 'genstrings: ' 'genstrings-logs.txt' | buiildkite-agent annotate --style error --context genstrings
fi

If that works, that could be an easy enough addition to the script without too much complication. If that doesn't work, then probably not worth spending too much time on it though.

Copy link
Contributor Author

@mokagio mokagio Nov 2, 2022

Choose a reason for hiding this comment

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

Thanks! I'll have a go a it.

I didn't go down the piping output option because I didn't want to lose the logs in the Buildkite UI. I had forgotten about the tee command 🤦‍♂️

Copy link
Contributor Author

@mokagio mokagio Nov 9, 2022

Choose a reason for hiding this comment

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

@AliSoftware addressed in 5e355c3.

What do you think of extracting this in the Buildkite plugin? I don't like how it depends on a lane name, which could vary across projects. But... we have a lot of convention over configuration already, and we can have proper error messages around that in the plugin command.

Copy link
Contributor

@AliSoftware AliSoftware Nov 9, 2022

Choose a reason for hiding this comment

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

Good idea to extract in plugin commands.

I agree that we have a lot of convention over configuration but I'm not sure the name of this lane is one that is currently consistent across repos at the moment 🤔 Maybe we can accept the lane name as optional parameter (${1:-generate_strings_file_for_glotpress}) to cover our backs (instead of failing if convention isn't followed in a repo)?

7 changes: 7 additions & 0 deletions .buildkite/pipeline.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,3 +123,10 @@ steps:
plugins: *common_plugins
agents:
queue: "android"
# Runs the `.strings` generation automation to ensure all the
# `LocalizedString` calls in the code can be correctly parsed by Apple's
# `genstrings`.
- label: "Lint Localized Strings Format"
Copy link
Contributor Author

@mokagio mokagio Nov 2, 2022

Choose a reason for hiding this comment

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

Keen on input on how to make this label shorter so it's not truncated with ellipsis in the Buildkite UI

image

💅

Copy link
Contributor

@AliSoftware AliSoftware Nov 2, 2022

Choose a reason for hiding this comment

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

I'm not feeling super inspired here tbh… Maybe Lint L10n Strings Format just so it can fit? 🤷

command: .buildkite/commands/lint-localized-strings-format.sh
plugins: *common_plugins
env: *common_env
4 changes: 2 additions & 2 deletions fastlane/lanes/localization.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@
#
# @called_by complete_code_freeze
#
lane :generate_strings_file_for_glotpress do
lane :generate_strings_file_for_glotpress do |options|
cocoapods

wordpress_en_lproj = File.join('WordPress', 'Resources', 'en.lproj')
Expand All @@ -141,7 +141,7 @@
destination: File.join(wordpress_en_lproj, 'Localizable.strings')
)

git_commit(path: [wordpress_en_lproj], message: 'Update strings for localization', allow_nothing_to_commit: true)
git_commit(path: [wordpress_en_lproj], message: 'Update strings for localization', allow_nothing_to_commit: true) unless options[:skip_commit]
end


Expand Down