Run genstrings in CI to validate LocalizedString usages

[mokagio]

The 21.0 code freeze was delayed because of a LocalizedString that didn't use strings literals and therefore failed the genstrings execution. See #19473.

The change had been in the codebase for a while, but we only found out at code freeze time.

To make the feedback loop faster, this PR adds a new CI step that runs the lane that generates the .strings to make sure everything runs fine.

Important Once this is merged into trunk, we ought to make the check a required one in the GitHub settings for trunk and release/*. I updated the internal document for this work, pdnsEh-Ly-p2, to remind me about it.

Implementation Note

This check requires a macOS agent, because of genstrings. We can run it in the “Build for Testing” step, before even starting the time consuming build, or in a dedicated step.

The first option saves resources, the second gives developers more feedback by means of showing whether the app builds and the tests pass regardless of the genstrings status.

Since we want to optimize for developer productivity, I opted for a dedicated step instead of making genstrings a precondition for "Build for Testing".

Testing

See #19544 which intentionally introduces a failure

Regression Notes

1. Potential unintended areas of impact – N.A.
2. What I did to test those areas of impact (or what existing automated tests I relied on) – N.A.
3. What automated tests I added (or what prevented me from doing so) – N.A.

---

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes. N.A.
• I have considered adding accessibility improvements for my changes. N.A.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

[mokagio]

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

[mokagio]

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

💅

[AliSoftware]

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

[peril-wordpress-mobile]

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

[mokagio]

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.

[AliSoftware]

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.

[mokagio]

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 🤦‍♂️

[mokagio]

@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.

[AliSoftware]

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)?

[wpmobilebot]

You can test the changes in Jetpack from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr19553-448d398 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[wpmobilebot]

You can test the changes in WordPress from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr19553-448d398 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[AliSoftware]

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 🙂

[mokagio]

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

Take the colored output in the following screenshots:

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.

[AliSoftware]

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.

[AliSoftware]

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.

[AliSoftware]

@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?

[mokagio]

@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.

[AliSoftware]

Side note that even with this in place, we might still miss issues with NSLocalizedString not using literals for value and comment, as were the case in #19606 (comment) — where there was no warning emitted by genstrings when I tried it on those problematic cases that happened there.

Not sure if there are other options / ideas to detect those kind of cases and to be sure to not miss any other.

[mokagio]

Not sure if there are other options / ideas to detect those kind of cases and to be sure to not miss any other.

The only thing I can think of is to be strict and require every *LocalizedString to have a string literal value (value: "..."). That might be possible via SwiftLint and a RegEx. Even though:

[AliSoftware]

@crazytonyli recently played around with SwiftSyntax and writing tools in Swift using it to do some linting on our code.

That use case of enforcing {NS,App}LocalizedString(…) to always have a literal value seems to me to be a good candidate for using SwiftSyntax in a similar way for this!