Update automation that freezes strings for translation #6742
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
mokagio
added
the
category: tooling
You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:
|
Notice with this change we lose the option to opt-out from committing. As far as I know, this is something we weren't currently using, and it's not something mentioned in the release management scenario. So, I think it's okay to drop it instead of reproducing it in the new version. For what is worth, WordPress Android doesn't do this either. https://github.com/wordpress-mobile/WordPress-Android/blob/6b2e279364d76077c9c96dbc9a15dfb615982d3f/fastlane/lanes/localization.rb#L363
mokagio
force-pushed
the
update-strings-freezing-process-part-3
branch
from
a3a7efa to
248e7bd
Compare
This makes it consistent with WordPress Android. See https://github.com/wordpress-mobile/WordPress-Android/blob/b0b7b348f50769c6d896283882d67658490f1dba/fastlane/lanes/release.rb#L59
Honest in the sense that it better represent what the lane does. The new name is `update_frozen_strings_for_translation`. This also makes it consistent with the naming from WordPress Android. See https://github.com/wordpress-mobile/WordPress-Android/blob/b0b7b348f50769c6d896283882d67658490f1dba/fastlane/lanes/release.rb#L60
mokagio
force-pushed
the
update-strings-freezing-process-part-3
branch
from
45c18c7 to
5b53d2a
Compare
There was a problem hiding this comment.
The code looks good… but when I then looked at your test PR in https://github.com/woocommerce/woocommerce-android/pull/6800/files, I realized that the diff of the strings.xml file didn't show any line gaining the a8c-src-lib XML attribute 🤔
This seems strange, because you seem to properly pass the source_id parameter to an_localize_libs, and are using the release toolkit 4.2.0 which already supports that param… so not sure what's happening here. But it's worth investigating to understand the root cause of this before merging this PR.
…ing-process-part-3
Finally had a chance to look into this. I basically compared how the action behaves for WordPress Android vs here. I noticed two things:
I'm not familiar with how localizing Android strings works so not sure where to go from here. The only thing I can think of is to edit if main_string_node.attr('tools:ignore').nil?
# No `tools:ignore` attribute; completely replace existing main string node with lib's one
add_xml_attributes!(lib_string_node, library) |
|
Haven't had time to dig deeper in this yet, but given the expected behavior described in paaHJt-3fg-p2 I believe there might indeed be a bug in the toolkit. I'd expect the strings coming from the libs to be annotated with the But I'll need to look deeper into this to be sure which path of the code this goes thru and what's really happening. Planned to do that today… but got a surprise hotfix to take care of so that shattered my plans for the day; hopefully I'll be able to check on my tomorrow 🤞 |
| }] | ||
| an_localize_libs(app_strings_path: MAIN_STRINGS_PATH, libs_strings_path: lib_to_merge) | ||
| File.delete(download_path) if File.exist?(download_path) | ||
| end | ||
| end | ||
|
|
||
| is_repo_clean = ("git status --porcelain").empty? |
There was a problem hiding this comment.
Lol I just realized that this old code could never have worked as intended anyway, because this used quotes instead of backticks (so tested if the "git status --porcelain" string was empty, which it isn't hence this always being false, as opposed to test the result of running the git command) 🤦
Thus explaining why @oguzkocer always saw the prompt when running this lane… even when there were no new strings to commit! (pdnsEh-rE-p2)
Glad that this PR will take the occasion to finally fix all that at once!
|
@mokagio any update on that? I have to admit I'm a bit swamped with my projects right now so haven't had the occasion to circle back on this to understand if this lack of |
|
@AliSoftware lol, I was waiting/hoping you could read that code and instantly figure out where the issue was. But rest assured I have not forgotten this. However, I cannot promise I'll get to it during this sprint. |
|
👋 @mokagio @AliSoftware ! Pinging you on this as it was accidentally shown in a PR view of mine as I was searching for something, then I got curious. This is just a friendly reminder in case it was buried on top of other priorities. Are we suppose to follow-up on that at some point, wdyt? 🤔 |
|
We definitively should follow-up on that and finally finish up that work—I think that was the last piece of the puzzle before finally considering the whole Localization Tooling Project Thread to be done, but we never had time to go back to it. @mokagio I think you said previously that you were planning to take point on finishing the investigation on this and get to the bottom of it to finally finish this PR? Personally my hands are pretty full with all the work needed in Tumblr anyway, so I'll let that to you if you don't mind 😓 |
|
Yeah nah. I have my hands full, too. I will get to this at some point, I suppose, but I'm not in a place to have the space to dig into the implementation details. FWIW and future reference, I did try to look into it in the more quiet period in December, and couldn't wrap my head around why it works in WordPress but not here. The only guess I have is that the inputs that WordPress was already formatted in such a way that met the expectation of the tool, while the one in Woo doesn't. I had two things I would have liked to try:
|
mokagio
added
status: do not merge
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #6742 +/- ##
=========================================
Coverage 44.55% 44.55%
Complexity 2969 2969
=========================================
Files 535 535
Lines 29418 29418
Branches 3884 3884
=========================================
Hits 13108 13108
Misses 15094 15094
Partials 1216 1216 ☔ View full report in Codecov by Sentry. |
Description
After the prep work from #6741 and #6703, this PR updates the automation to freeze the strings for translation.
Notably it:
source_idparameter to differentiate the generated stringsTesting instructions
To test this, I created a new branch from this one, hacked the
complete_code_freezelane to work outside of a release branch, and used it to verify the process succeeds. You can see it, including the commit with the new frozen strings, in #6800, but for reference here's the lane outputRELEASE-NOTES.txtif necessary. – N.A.