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
mokagio 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 all 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
43 changes: 43 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,43 @@
#!/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"
LOGS=logs.txt
set +e
set -o pipefail
bundle exec fastlane generate_strings_file_for_glotpress skip_commit:true | tee $LOGS
EXIT_CODE=$?
set -e

echo $EXIT_CODE

if [[ $EXIT_CODE -ne 0 ]]; then
# Strings generation finished with errors, extract the errors in an easy-to-find section
echo "--- Report genstrings errors"
ERRORS=errors.txt
echo "Found errors when trying to run \`genstrings\` to generate the \`.strings\` files from \`*LocalizedStrings\` calls:" | tee $ERRORS
echo '' >> $ERRORS
# Print the errors inline.
#
# Notice the second `sed` call that removes the ANSI escape sequences that
# Fastlane uses to color the output.
grep -e "\[.*\].*genstrings:" $LOGS \
| sed -e 's/\[.*\].*genstrings: error: /- /' \
| sed -e $'s/\x1b\[[0-9;]*m//g' \
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.

What about moving this one sed on the line that pipes to buildkite-agent annotate instead? That way we'd still have colors included in the tee'd logs sent to stdout and only remove the ANSI codes from the annotation 🙂

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.

Unfortunately, without that filter, we get a double match for one of the errors.

Take the colored output in the following screenshots:

image

When run through the code without the ANSI color RegEx, it returns

Found errors when trying to run `genstrings` to generate the `.strings` files from `*LocalizedStrings` calls:

- bad entry in file WordPress/Classes/Extensions/Colors and Styles/WPStyleGuide+Search.swift (line = 50): Argument is not a literal string.
- bad entry in file WordPress/Classes/Extensions/Colors and Styles/WPStyleGuide+Search.swift (line = 50): Argument is not a literal string.
- bad entry in file WordPress/Classes/ViewRelated/Aztec/Extensions/Header+WordPress.swift (line = 28): Argument is not a literal string.

As far as I can tell, the reason is because of different terminators between the errors printed in line and the ones at the end of the Fastlane execution. With cat -e, we can see

[05:19:23]: ^[[32m-------------------------------------------------^[[0m$
[05:19:23]: ^[[32m--- Step: ios_generate_strings_file_from_code ---^[[0m$
[05:19:23]: ^[[32m-------------------------------------------------^[[0m$
[05:19:23]: �M-^V� ^[[35mgenstrings: error: bad entry in file WordPress/Classes/Extensions/Colors and Styles/WPStyleGuide+Search.swift (line = 50): Argument is not a literal string.^[[0m$
[05:19:23]: �M-^V� ^[[35mgenstrings: error: bad entry in file WordPress/Classes/ViewRelated/Aztec/Extensions/Header+WordPress.swift (line = 28): Argument is not a literal string.^[[0m$
+------------------+-----------------------------------------+$
|                        ^[[33mLane Context^[[0m                        |$
+------------------+-----------------------------------------+$
| DEFAULT_PLATFORM | ios                                     |$
| PLATFORM_NAME    | ios                                     |$
| LANE_NAME        | ios generate_strings_file_for_glotpress |$
+------------------+-----------------------------------------+$
[05:19:26]: ^[[31mgenstrings: error: bad entry in file WordPress/Classes/Extensions/Colors and Styles/WPStyleGuide+Search.swift (line = 50): Argument is not a literal string.$
genstrings: error: bad entry in file WordPress/Classes/ViewRelated/Aztec/Extensions/Header+WordPress.swift (line = 28): Argument is not a literal string.^[[0m$
[05:19:26]: ^[[31mfastlane finished with errors^[[0m$

You can see that the second bad entry in file WordPress/Classes/Extensions/Colors and Styles/WPStyleGuide+Search.swift (line = 50)... doesn't have the ^[[0m$ suffix. I'm guessing that's because Fastlane prints all the errors it collected in a single colored expression, but with new lines. The fact that there's only one [05:19:26] timestamp annotation for both errors at the end of the logs points to that, too.

I'm sure there's a RegEx we can write to work around all this, but it sounds like we'd be hitting the diminishing return threshold.

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.

Ah yeah, I see. Tricky. Nice catch.

Let's keep the filter to remove ANSI code early in the command like you did then.

Besides, even if we managed to keep the colors while still properly removing duplicates, I don't think there's any case where those error lines use multiple styles (e.g. multiple colors / boldness / … within the line to emphasis some words), and instead all the lines seems to all be in a single style each… either all red or all purple. So we wouldn't end up benefit from keeping the coloring much (and even if we wanted those to appear red in the the stdout of the dedicated section to make them stand out, we could always re-add the red-color-ANSI-code around it all manually ourselves 🤷 ). So not worth keeping the initial ANSI-codes after all indeed.

Copy link
Contributor

@AliSoftware AliSoftware Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Tbh, the nicer solution would probably be to have generate_strings_file_for_glotpress handle the "write-errors-to-file" capability:

  • Either by providing the right options or indirections from the implementation of the action itself when it calls genstrings (something like genstrings -o Localizable.strings $files 2>genstrings-stderr.log—or maybe something a bit more convoluted to use tee, but on stderr and not stdout, if genstrings prints those warnings to stdout?—, where the genstrings-stderr.log file name would be provided by a ConfigItem defaulting to nil?), which would probably be the cleanest solution, but would require updating the action itself in release-toolkit to add such support
  • Or by updating the generate_strings_file_for_glotpress lane in your Fastfile to redirect just the output of the call to ios_generate_strings_file_from_code action into a log file (instead of the output of the whole fastlane invocation, which includes that but also all the ceremony with lane switch header, Lane Context table, and summary lines at the end)

And then parse that file to detect and extract the genstrings warnings, without those parsed logs being cluttered by other fastlane logs.

Only drawback I see from that is that the plugin command (that this script would be turned into) would have to rely on you not only have a generate_strings_file_for_glotpress lane, but also for you to have it invoke ios_generate_strings_file_from_code with the proper log file name to write to, so that means relying a lot on conventions being followed.

Copy link
Contributor

@AliSoftware AliSoftware Nov 16, 2022

Choose a reason for hiding this comment

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

@mokagio How do you want to go forward with this PR?

  • Address the above feedback (i.e. move the "write to file redirect" to just the call on generate_strings_file_for_glotpress instead of the whole fastlane run, or update our action in release toolkit to add some ConfigItem(key: :errors_log_file) to it etc) in this PR?
  • Or merge as is, and address this improvement in a follow-up PR?
  • Or merge as is, and migrate that logic to a dedicated command in bash-cache in follow-up PRs, improving it / addressing above feedback in there later too?

Copy link
Contributor Author

@mokagio mokagio Nov 17, 2022

Choose a reason for hiding this comment

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

@AliSoftware thanks for asking.

Or merge as is, and migrate that logic to a dedicated command in bash-cache in follow-up PRs, improving it / addressing above feedback in there later too?

It's, sort of, the best of both worlds. We have something that works and we can ship ASAP, then we can iterate from a centralized place.

I say "sort of", because if we go down the path of updating the toolkit, we either use a default value for the log files or we'd have to update all the clients again.

Still, give this has the possibility of preventing some issue, I'd say let's ship as is ASAP to get more coverage, then improve on the setup.

| sort \
| uniq \
| tee -a $ERRORS
# Annotate the build with the errors
cat $ERRORS | buildkite-agent annotate --style error --context genstrings
fi

exit $EXIT_CODE
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