[Tooling] Add linter for AppLocalizedString not used in extension targets by AliSoftware · Pull Request #18427 · wordpress-mobile/WordPress-iOS

AliSoftware · 2022-04-22T19:45:14Z

This work is part of paaHJt-2Ib-p2, and a follow up of the annoucement in paNNhX-nP-p2.

What

Adds a linter which checks that every source file included in an app extension target (e.g. Widgets, NotificationExtension, ShareExtension…) uses AppLocalizedString instead of NSLocalizedString, to be sure it uses the translations from the app bundle (as we put all our Localizable.strings file in the app bundle to share between them app and extension targets, and reduce the overall .ipa size)

This also fixes 3 of such violations that the linter found in the process.

Screenshot of the linter in action

Another example on a file used in 4 different app extension targets

Related Past PRs

For reference, the following past PRs merged a while ago migrated call sites of NSLocalizedString to AppLocalizedString for files used in extension targets.

This is why the linter that this PR adds today only found 3 violations (introduced after these PRs landed) and not hundreds.

To Test

🔨 Build the WordPress target in Xcode, and verify that there is no error (CI will already do that for you)
✏️ Edit the WordPress/Classes/ViewRelated/Stats/Extensions/Double+Stats.swift file and replace one of the AppLocalizedString calls it uses with NSLocalizedString
🔨 Build the WordPressShareExtension scheme, and verify that the build fails and the error is shown in on the right line.
- 🕵️ There might be multiple errors on the same line (one for each extension target complaining about it) if that file is build for multiple extension targets
- 🕵️ For example, when building the WordPressShareExtension scheme, Xcode actually also builds the WordPress app target (given how that scheme is configured see Edit Scheme > Build tab), which in turns builds other app extensions that the .app will embed in its bundle. Hence potentially many more targets being built depending on the schemes, and thus more instances of the error
💻 Optional: run Scripts/BuildPhases/LintAppLocalizedStringsUsage.rb in your Terminal and observe that it will scan all the App Extension targets found in the project — and report the same errors as the ones in Xcode — and exits with status code 1 (echo $?)
✏️ Fix the violation you introduced (i.e. revert the call you changed)
🔨 Start a build again, and verify it still passes.

AliSoftware · 2022-04-22T19:48:17Z

 				C737554027C80F1300C6E9A1 /* String+CondenseWhitespace.swift in Sources */,
 				7335AC6D21220F0F0012EF2D /* FormattableUserContent.swift in Sources */,
 				7335AC6221220E690012EF2D /* FormattableContentFactory.swift in Sources */,
+				09DBEA55281336E10019724E /* AppLocalizedString.swift in Sources */,


This is me adding that AppLocalizedString.swift file (the one containing the implementation for the AppLocalizedString function) to the WordPressNotificationServiceExtension target, so I can call the function in WordPress/WordPressNotificationServiceExtension/Sources/NotificationService.swift below

AliSoftware · 2022-04-22T19:55:15Z

+			outputPaths = (
+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = "/bin/zsh -euo pipefail";


Using zsh as the shell here (instead of sh) intentionally, because:

This calls a Ruby script

Which thus depends on the user's configuration of rbenv or rvm or similar

And those tools are usually set up in ~/.zshrc

So using /bin/sh here would not read the user's ~/.zshrc, thus not have rbenv properly set up, and thus might not use the expected version of Ruby or fail.

🤔 this assumes every developer uses ZSH and configured a Ruby version manager. I think it's reasonable to expect folks to use rbenv or rvm since they would likely get issues with the repo if they didn't, but I don't know how Fish users (if any) might deal with this change 🤔

I suppose one option is to "just ship it" and see if someone complains about it?

Or... could we use /usr/bin/env ruby directly? 🤔

🤔 ❗ ❗

The script failed for me because it used system Ruby (how?) and (something I didn't realize when reading the code) xcodeproj is not available there.

In fact, assuming xcodeproj to be available in any user Ruby install doesn't seem safe. Folks would have it available locally to this project via Bundler, but they might not have ever had the need to run gem install xcodeproj.

but they might not have ever had the need to run gem install xcodeproj.

I assumed that was true because we use Bundler and furthermore vendor our gems. Then I doubted myself... But, I just verified it by:

Uninstalling fastlane and xcodeproj from my Ruby 2.7.6

Running bundle install

Checking if xcodeproj is available – It no longer is

Running the script manually – It fails

Or... could we use /usr/bin/env ruby directly? 🤔

No, that doesn't make sense because then the content of the shellScript would have to be actual Ruby code, but we need it DRY and in a script. (I tried that here...)

I think a next step might be to wrap the Ruby script call into a .sh script that uses bundle exec ruby ... and does all the necessary checks beforehand.

Ahhh dammit I was afraid such case and portability issue would happen 😭
Not the first time I encounter the question of "How to make a script called by Xcode use the user's shell setup, especially so it knows about Ruby and Bundler". This is tricky because:

We should probably not assume which shell the users use (like I did here with zsh, considering that because zsh was now the default on macOS, this would be a safe bet… but that discards anyone who decided to use an alternate shell like fish indeed.

Though to be honest personally if I were to use fish I'd probably organize my rc files so that zsh still works and configure ruby and bundler, and then my fish rc would source that ~/.zshrc (and possibly add fish-specific stuff after that), that way zsh would still work as well even if my everyday shell were fish…

…but eh, that's another assumption I can't really make to ensure this is really portable

We can't use /usr/bin/env ruby either here, because that risks not only using the system ruby, but also not using bundler (and thus picking up the xcodeproj gem installed in the project's bundle, as part of a cocoapods dependency anyway)

To be fair my current solution didn't call bundler either, as you noticed. That worked on my setup because I have a plugin configured in ~/.zshrc that automatically prepends bundle exec to many known gems and executions or ruby scripts by default for me, which is probably why I missed that when working on my PR

But that's obviously yet again not an assumption we can make

I'll try your suggestion of making the Script Build Phase use /bin/sh -euo pipefail, and it just calling a .sh script that would wrap the calls to bundle exec and to the ruby script.

But tbh I'm not super confident that this will work perfectly either, because the .sh shell spawned by Xcode is a child process of Xcode, not of the login shell and Terminal, so it would not depend on (nor inherit the setup of) the user's default shell (the one they use when launching their Terminal.app). Such a child sh shell spawned by Xcode would thus not have knowledge of the setup and config related to bundler and rbenv the user usually have for their default shell in Terminal…

And obviously we can't either make the our .sh wrapper script assume anything about the user's setup; so the potential ideas of configuring rbenv from that wrapper script (e.g. calling eval $(rbenv init -) or export PATH=~/.rbenv/shims:$PATH etc) won't work either, as users might be using rvm instead of rbenv in their system…

I've found this post which explains the issue (which confirms what I've encountered with this kind of cases in the past), and also provides some ideas… but it also relies on the assumption that the user a particular Ruby Version manager (rvm in that post's case)… so still not a universal solution.

Maybe I'll have to make my wrapper script just test which Ruby Version Manager tool is installed (test which rbenv / command -v rbenv vs command -v rvm and act accordingly) and cover both cases… (and here goes my plan for the day 😅 )

@mokagio It took me most of the day to figure out a working (?) solution that should cover cases of folks using any shell, and folks using either rbenv or rvm, but the last 2 commits should finally work.

At least @jhnstn helped testing the setup of using rvm, and @twstokes helped testing the setup of using rbenv (see Slack thread in p1651001863417419/1650990034.879279-slack-C011BKNU1V5) and after a lot of trial and error tweaking my script to make it work, this finally seem to cover all setups properly 🤞

wpmobilebot · 2022-04-22T20:10:19Z

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 pr18427-f16f06d on your iPhone

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

wpmobilebot · 2022-04-22T20:11:06Z

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 pr18427-5570dab on your iPhone

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

- Script Build Phase added to each of the targets that were of type App Extension - In each of those target, the Script Build Phase just calls `Scripts/BuildPhases/LintAppLocalizedStringsUsage.rb` - That script will scan each source file that is built as part of said extension target and check if `NSLocalizedString` appears used in any of them, and generate an error if so, suggesting to use `AppLocalizedString` instead.

AliSoftware · 2022-04-22T21:02:17Z

Enabling auto-merge because this PR touches the .pbxproj, so the sooner it lands before diverging too much with trunk the less chances we'll get conflicts on the file 😛

`pod install` made the file references be reordered alphabetically, which is nice… but is more likely to cause conflicts when the `pbxproj` file is changed in `trunk` while this PR is still pending

mokagio

I wasn't able to verify the behavior locally, which I think points out to more work being needed to make this portable for a variety of devs setups.

See comments here.

mokagio · 2022-04-26T04:19:29Z

+			outputPaths = (
+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = "/bin/zsh -euo pipefail";


🤔 this assumes every developer uses ZSH and configured a Ruby version manager. I think it's reasonable to expect folks to use rbenv or rvm since they would likely get issues with the repo if they didn't, but I don't know how Fish users (if any) might deal with this change 🤔

I suppose one option is to "just ship it" and see if someone complains about it?

Or... could we use /usr/bin/env ruby directly? 🤔

mokagio · 2022-04-26T04:32:50Z

+			outputPaths = (
+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = "/bin/zsh -euo pipefail";


🤔 ❗ ❗

The script failed for me because it used system Ruby (how?) and (something I didn't realize when reading the code) xcodeproj is not available there.

In fact, assuming xcodeproj to be available in any user Ruby install doesn't seem safe. Folks would have it available locally to this project via Bundler, but they might not have ever had the need to run gem install xcodeproj.

mokagio · 2022-04-26T04:40:04Z

+			outputPaths = (
+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = "/bin/zsh -euo pipefail";


but they might not have ever had the need to run gem install xcodeproj.

I assumed that was true because we use Bundler and furthermore vendor our gems. Then I doubted myself... But, I just verified it by:

Uninstalling fastlane and xcodeproj from my Ruby 2.7.6

Running bundle install

Checking if xcodeproj is available – It no longer is

Running the script manually – It fails

mokagio · 2022-04-26T05:08:31Z

+			outputPaths = (
+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = "/bin/zsh -euo pipefail";


Or... could we use /usr/bin/env ruby directly? 🤔

No, that doesn't make sense because then the content of the shellScript would have to be actual Ruby code, but we need it DRY and in a script. (I tried that here...)

I think a next step might be to wrap the Ruby script call into a .sh script that uses bundle exec ruby ... and does all the necessary checks beforehand.

Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>

To make sure rbenv / rvm are set up if used by the user, and that the script is run via `bundle exec ruby <path>` so that the ruby script would also be able to reference gems (like `xcodeproj`) from the local bundle.

AliSoftware · 2022-04-26T19:56:39Z

+		};
+		09DBEA4D281333060019724E /* [Lint] Check AppLocalizedString usage */ = {
+			isa = PBXShellScriptBuildPhase;
+			alwaysOutOfDate = 1;


Used this (aka un-checking the "Based on dependency analysis" checkbox on Xcode's UI for the Build Phase) since it's not really possible to specify proper "Input Files" for that build phase to properly guess when it should run or not — after all, the reason why we have to rely on the ruby script and using the xcodeproj gem is to know which of those input files to lint in the first place! — so we we don't have much other option to tell Xcode to run it in all cases…

AliSoftware · 2022-04-26T20:00:16Z

+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = "/bin/bash -eu";
+			shellScript = "\"$SRCROOT/../Scripts/BuildPhases/runRubyScript\" \"LintAppLocalizedStringsUsage.rb\" \"${TARGET_NAME}\"\n";


This is the new way I used to wrap the call to the script into proper ceremony to setup rbenv/rvm/bundle exec for its execution.

I could have just made it a RunLintAppLocalizedStringsUsage.sh script specific for that one, which would have the call to bundle exec ruby LintAppLocalizedStringUsage.rb "$TARGET_NAME" hardcoded in it, but I figured it could be useful to have something more generic like runRubyScript which could wrap any arbitrary ruby script, so that if in the future we need to add other Build Phases based on ruby scripts, this could be reused (and also, if that works well, I might steal it for some other personal OSS projects that might also use ruby scripts for script build phases… 😇 )

+1. I like that this is generic 👍

AliSoftware · 2022-04-26T20:07:27Z

+#
+
+# Add `rbenv` and `rvm` binaries to PATH, so that we support both
+export PATH="$HOME/.rbenv/shims:$HOME/.rvm/bin:$PATH"


At first I tried to add some logic to detect which of rbenv or rvm were installed, and call the setup commands appropriately for each and all (see attempt in parent commit a84b805) but that didn't work. E.g. command -v rbenv apparently failed to detect that rbenv was installed, even system-wise, when ran by Xcode — while it worked in Terminal — so it didn't go into the if in Tanner's Mac… (even though on my own Mac it did work)

So in the end I settled for a simpler solution after all, which is to add the paths to the shims and binaries for both cases in $PATH here to cover both bases, and then calling bundle exec ruby <script.rb> — which would, in both cases (rvm like rbenv), use the shims and thus the locally installed ruby instead of the system one (while I think that just bundle exec <script.rb> would have used the shebang at the to of the script.rb, and in such case I'm not sure which one /usr/bin/env ruby would end up using…? 🤷 )

@mokagio

See @mokagio's suggestion in #18427 (comment)

mokagio · 2022-04-27T03:56:52Z

+echo "Running the script using 'bundle exec' ..."
+cd "$(dirname "${BASH_SOURCE[0]}")"
+bundle exec ruby "$@"
+cd -


Could this cd - be omitted, given the script runs in its own ephemeral shell, or is it useful to have it here?

Oh it could have been omitted indeed. It's just my muscle memory always balancing cd xyz with cd - for good etiquette that got the best of me not thinking it was indeed unnecessary in that case 😅

mokagio · 2022-04-27T04:07:00Z

+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = "/bin/bash -eu";
+			shellScript = "\"$SRCROOT/../Scripts/BuildPhases/runRubyScript\" \"LintAppLocalizedStringsUsage.rb\" \"${TARGET_NAME}\"\n";


+1. I like that this is generic 👍

mokagio

Run locally and it worked 🎉

I'm going to approve so auto-merge kicks in and we can avoid conflicts in the project file 🤞

But, I have a curve ball to throw at you... Yesterday I was chatting with @jkmassel and he refreshed in my mind the vision of making the day-to-day work for product developers Ruby-less.

This new big of Ruby tooling goes in the opposite direction 😳 I wish I realized it when I saw the PR. Obviously, we are far from the point where we can no longer depend on Ruby, but that day will come eventually.

Any ideas how we could rewrite this script in the future to not use Ruby? Maybe Swift has some utility to parse the project file that we can adopt in place of the xcodeproj gem?

AliSoftware · 2022-04-27T08:25:31Z

Any ideas how we could rewrite this script in the future to not use Ruby? Maybe Swift has some utility to parse the project file that we can adopt in place of the xcodeproj gem?

@mokagio I think there has been a port of the xcodeproj gem into a Swift package by the team behind Tuist (at least it would make sense for them to need such port anyway). But that would mean writing this phase as a Swift script… that would use SPM dependencies (as opposed to being a standalone Swift script we could run directly using the swift interpreter), so we'd have to make it a small project that we'd then compile into an executable and statically linked with the Xcodeproj swift lib etc. Probably feasible but might require some non-trivial setup compared to having a script where we can read the source directly.

Another alternative would be to have the swift script not depend on the Xcodeproj package nor any other dependency and instead parse the project file directly (pbxproj files are ASCII Plists so we can Dictionary(contentOf: file) on them). That would be a bit more convoluted to then interpret the resulting dictionary than if it was already transformed into a viewmodel representing a project with easier methods to manipulate its content, but since our goal would be very specific and narrow (find the entry for a target, then the compile Build phase, then the file references it contains, then their paths from the object list) it might not be that complicated after all (famous last words 😅)

mokagio · 2022-04-28T00:45:27Z

Nice. This is useful info to look at when we'll be seriously considering getting rid of any Ruby dependency on the devs side 👍

AliSoftware · 2022-04-28T03:36:43Z

Couldn't resist the nerd snipe 😅 #18462

(tbh I kinda jumped on the occasion as a good excuse to do some swift code, as it was a long time and I missed it 😇; was good fun and refreshing! 🙂)

AliSoftware added the Testing Unit and UI Tests and Tooling label Apr 22, 2022

AliSoftware added this to the 19.8 milestone Apr 22, 2022

AliSoftware self-assigned this Apr 22, 2022

AliSoftware force-pushed the tooling/applocalizedstring-linter branch from b742b3f to 9c50d64 Compare April 22, 2022 19:46

AliSoftware requested review from a team and mokagio April 22, 2022 19:48

AliSoftware commented Apr 22, 2022

View reviewed changes

AliSoftware mentioned this pull request Apr 22, 2022

[Tooling] Add Swiftlint rule for NSLocalizedString -> AppLocalizedString #17677

Closed

AliSoftware added 2 commits April 22, 2022 22:24

Fix violations found by the linter

3b484b6

AliSoftware force-pushed the tooling/applocalizedstring-linter branch from 9c50d64 to 3b484b6 Compare April 22, 2022 20:24

AliSoftware enabled auto-merge April 22, 2022 21:02

AliSoftware commented Apr 24, 2022

View reviewed changes

Comment thread Scripts/BuildPhases/LintAppLocalizedStringsUsage.rb Outdated

AliSoftware added 2 commits April 24, 2022 03:47

Update Scripts/BuildPhases/LintAppLocalizedStringsUsage.rb

a4f2599

Revert file references moved around in pbxproj

ce4e6f5

`pod install` made the file references be reordered alphabetically, which is nice… but is more likely to cause conflicts when the `pbxproj` file is changed in `trunk` while this PR is still pending

mokagio reviewed Apr 26, 2022

View reviewed changes

AliSoftware and others added 3 commits April 26, 2022 11:46

Use .each instead of .map in script

4d796df

Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>

Use runRubyScript wrapper on Script Build Phase

a84b805

To make sure rbenv / rvm are set up if used by the user, and that the script is run via `bundle exec ruby <path>` so that the ruby script would also be able to reference gems (like `xcodeproj`) from the local bundle.

Alternate idea: configure both cases without trying to auto-detect

ec7f727

AliSoftware requested a review from mokagio April 26, 2022 19:52

AliSoftware commented Apr 26, 2022

View reviewed changes

Adding a message at end of linting each target

5570dab

See @mokagio's suggestion in #18427 (comment)

mokagio reviewed Apr 27, 2022

View reviewed changes

Remove typo from comment in runRubyScript

f16f06d

mokagio approved these changes Apr 27, 2022

View reviewed changes

AliSoftware merged commit 3097c99 into trunk Apr 27, 2022

AliSoftware deleted the tooling/applocalizedstring-linter branch April 27, 2022 05:09

AliSoftware mentioned this pull request Apr 28, 2022

[Tooling] Re-implement AppLocalizedStrings linter in swift #18462

Merged

Uh oh!

Conversation

AliSoftware commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Screenshot of the linter in action

Related Past PRs

To Test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Apr 22, 2022

Uh oh!

Uh oh!

mokagio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio commented Apr 28, 2022

Uh oh!

AliSoftware commented Apr 28, 2022

Uh oh!

Reviewers

Assignees

Labels

AliSoftware commented Apr 22, 2022 •

edited

Loading

wpmobilebot commented Apr 22, 2022 •

edited

Loading

wpmobilebot commented Apr 22, 2022 •

edited

Loading

mokagio left a comment •

edited

Loading

AliSoftware Apr 26, 2022 •

edited

Loading

AliSoftware commented Apr 27, 2022 •

edited

Loading