Move Fastlane to Project Root #15706

jkmassel · 2021-01-26T00:13:05Z

Aligns this project with others to put the fastlane directory at the root of the project.

To Test:
Note that the following CI jobs are passing for this PR:

Build Tests
Unit Tests
UI Tests (both)
Installable Build
Translation Build (note that the build in appetize.io is the one associated with the CI run for this PR)

To Review:
This is (necessarily) a huge PR because it moves every file in the fastlane directory. I'd recommend reviewing this by commit – the first just moves all of the files into the new location – subsequent ones alter them to make them compatible with the new location.

It's possible that there might be other breaking changes I haven't encountered while testing, but I'd like to land this in the release branch (then in develop) as soon as possible in order to avoid conflicts with other changes – I can fix other broken tooling if/when I run into it – this should be the vast majority of it.

--

Reviews: I'd like review from both @AliSoftware and @loremattei for this – @loremattei because he's most familiar with all of it and @AliSoftware because I streamlined a bunch of the pathing and variables and I'd love your input on the changes.

Thank you!!

peril-wordpress-mobile · 2021-01-26T00:13:17Z

You can trigger an installable build for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-01-26T00:31:11Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

AliSoftware

Only initial review for now (by just reading diff from GitHub per commit; not tested locally yet)

.gitignore

Scripts/extract-framework-translations.swift

Scripts/update-translations.rb

fastlane/Fastfile

Co-authored-by: Olivier Halligon <[email protected]>

loremattei

I gave a first pass and left a few comments.

I'm going to be AFK for the rest of this week, but I don't want to block the work on this PR, so I've put a [**] on the two that I think can lead to bugs and should be addressed or carefully tested before merging.

For the others, feel free to do what you prefer.

Scripts/update-translations.rb

loremattei · 2021-02-02T10:00:41Z

Scripts/update-translations.rb

  system "grep -a '\\x00\\x20\\x00\\x22\\x00\\x22\\x00\\x3b$' #{lang_dir}/Localizable#{strings_file_ext}.strings"
+
+  # Clean up after ourselves
+  FileUtils.rm destination if File.exist? destination


[**] I'm not sure about this. I think this has been added to replace the call to system "rm #{lang_dir}/Localizable#{strings_file_ext}.strings.bak" which has been removed from above, right?

If so, I think we should add .bak to destination, otherwise it looks like to me that this will delete the strings files instead of the temporary backups.

Resolved in 7f6cded!

loremattei · 2021-02-02T10:45:29Z

fastlane/Fastfile

 ENV["DOWNLOAD_METADATA"]="./fastlane/download_metadata.swift"
-ENV["PROJECT_ROOT_FOLDER"]="../"


[**] I'm not sure we can get rid of passing PROJECT_ROOT_FOLDER as an env variable 🤔 IIRC this is used in the release-toolkit as well.

It is indeed, well spotted.

Though now that all our projects are aligned to have the fastlane/ folder at root after this change, we could consider make the release-toolkit default that value to ../, or better yet, compute it – by move up the dir hierarchy in a while loop until we find .git/ folder, or by just using git rev-parse --show-toplevel. But that's for yet another bullet point that I'll have to add to my long refactoring todo list, so we're not there yet in release-toolkit and we should keep the env var for now.

Ah ok – easy enough to fix – I'll just set it for now then we can remove it in the Great Refactor ™

ref: 38c660e

Tracked in wordpress-mobile/release-toolkit#205

loremattei · 2021-02-02T10:51:04Z

fastlane/Fastfile

      export_method: "enterprise",
      clean: true,
-      output_directory: "../build/",
-      derived_data_path: "../derived-data/alpha/",


With that we're moving the derived_data_path to DerivedData for both the Internal and the Beta 🤔
I can't think about issues with this, but wanted to highlight to make sure this is wanted.

Yeah, I thought it was weird that we were using different directories for each - especially locally it sort of makes the project directory messy.

I don't foresee any issues with this (especially given that we have clean: true there – it'll do cleanup prior to the build locally.

loremattei · 2021-02-02T10:52:55Z

fastlane/Fastfile

-      export_options: { method: "enterprise" })
-
-    sh("mv ../../build/WordPress.ipa \"../../build/WordPress Alpha.ipa\"")
+      export_options: { method: "enterprise" }


This looks good to me on CI, but I found it useful to be able to run the lane locally to build both the versions and have both of them available at the end.

Hmm good point – I might make it so that this just moves them to the Artifacts directory that we already have – that way it's consistent across the whole project – if you want the .ipa file locally, it'll always be in Artifacts (which is what we use for the translation lane)

This is adjusted in b15122e, which puts the build artifacts for all lanes in the Artifacts directory.

loremattei · 2021-02-02T10:53:32Z

fastlane/Fastfile

      export_team_id: get_required_env("INT_EXPORT_TEAM_ID"),
-      export_options: { method: "enterprise" })
-
-    sh("mv ../../build/WordPress.ipa \"../../build/WordPress Internal.ipa\"")


Same as above

This is adjusted in b15122e, which puts the build artifacts for all lanes in the Artifacts directory.

jkmassel · 2021-02-03T21:02:36Z

@AliSoftware this is ready for another look when you are!

# Conflicts: # fastlane/metadata/ar-SA/release_notes.txt # fastlane/metadata/de-DE/release_notes.txt # fastlane/metadata/default/release_notes.txt # fastlane/metadata/en-GB/release_notes.txt # fastlane/metadata/en-US/release_notes.txt # fastlane/metadata/es-ES/release_notes.txt # fastlane/metadata/es-MX/release_notes.txt # fastlane/metadata/fr-FR/release_notes.txt # fastlane/metadata/id/release_notes.txt # fastlane/metadata/it/release_notes.txt # fastlane/metadata/ja/release_notes.txt # fastlane/metadata/ko/release_notes.txt # fastlane/metadata/nl-NL/release_notes.txt # fastlane/metadata/ru/release_notes.txt # fastlane/metadata/sv/release_notes.txt # fastlane/metadata/tr/release_notes.txt # fastlane/metadata/zh-Hans/release_notes.txt # fastlane/metadata/zh-Hant/release_notes.txt

AliSoftware

The recurring theme when reviewing this PR from the GitHub UI is clearly this 😅

Joke aside, added a couple of nits, but also saw a couple of files that were commited while they really shouldn't.

It can easily be seen by looking at the diff between 16.6.0.1 (the tag from which you started to initially cut the branch from at that time, iinm) and move/fastlane – after cloning the branches locally (in Terminal, because a diff in GitHub UI will be swamped by the diff size) – like so: git diff --summary 16.6.0.1 move/fastlane | grep -v rename

.circleci/config.yml

Scripts/update-translations.rb

fastlane/Fastfile

fastlane/README.md

.gitignore

AliSoftware · 2021-02-09T19:15:56Z

Note: I've created a Draft P2 post in +iosp2 (edit link sent in DM – ref: paNNhX-c0-p2) – just so we don't forget to announce the change made by that PR – that is ready to be published once we're ready to approve and merge this PR.

loremattei

This looks generally good to me, other than the accidentally added files that Olivier pointed out.
I've still a doubt about the refactoring of update-translations.rb, so I left a question there.

I've tried to run a couple of modified lanes and they worked as expected anyway :-)

Scripts/update-translations.rb

Co-authored-by: Olivier Halligon <[email protected]>

jkmassel · 2021-02-11T20:41:37Z

Thanks for the feedback @loremattei and @AliSoftware – it should all be addressed.

I did need to adjust one thing – Ruby wasn't happy with:

system("curl", "-fgsLo", destination, url) || puts "Error downloading #{url}"

but it was fine with

  system("curl", "-fgsLo", destination, url) or raise "Error downloading #{url}"

for reasons I don't entirely understand.

That said, I think I prefer this, because IMHO if it can't download the strings it should fail the build instead of carrying on with missing strings.

loremattei

LGTM!
I briefly tested update-translation.rb and it worked as expected. I recommend double checking the diff it generates on the first actual release.

AliSoftware

The merge of release/16.7 into that branch made it a bit hard to ensure the last diff was right, but looking at the incremental diffs from last time (537 commits behind 😱) it LGTM.

…
Switched to branch 'move/fastlane'
Your branch is behind 'origin/move/fastlane' by 537 commits, and can be fast-forwarded.

So I say and:

Don't forget to publish the draft P2 post I prepared [paNNhX-c0-p2] to announce the change (even if we MIEs might be more frequent users of those lanes than the code wranglers)
As Lore said, we might want to remember to double-check the diff during next release just to be sure. (It will always be time to fix anything we might have missed later anyway if needed, so better finally merge it ASAP)

Move fastlane to the project root

fc80c04

jkmassel self-assigned this Jan 26, 2021

jkmassel added the Tooling Build, Release, and Validation Tools label Jan 26, 2021

jkmassel force-pushed the move/fastlane branch 8 times, most recently from d229677 to 0920e4f Compare January 30, 2021 06:50

jkmassel added 2 commits January 29, 2021 23:51

Update Tooling to use new paths

3397b7c

Fix translation builds

b00b031

jkmassel force-pushed the move/fastlane branch from 0920e4f to b00b031 Compare January 30, 2021 06:53

jkmassel requested review from AliSoftware and loremattei January 30, 2021 06:54

jkmassel marked this pull request as ready for review January 31, 2021 07:13

AliSoftware reviewed Feb 1, 2021

View reviewed changes

AliSoftware self-requested a review February 1, 2021 17:19

jkmassel and others added 6 commits February 1, 2021 14:57

Pathing fix

71959eb

Co-authored-by: Olivier Halligon <[email protected]>

Use safe system calls

bb46c20

Ensure the PROJECT_ROOT_FOLDER is always correct

32241c0

Clean up sh usage

868f344

Use proper path building for itc lane

42ad53a

Better path safety in Swift, too

9ecb9f5

jkmassel added this to the 16.6 ❄️ milestone Feb 2, 2021

loremattei reviewed Feb 2, 2021

View reviewed changes

Ensure ENV[“PROJECT_ROOT”FOLDER”] is set

38c660e

jkmassel requested a review from loremattei February 3, 2021 20:32

jkmassel modified the milestones: 16.6, 16.7 ❄️ Feb 8, 2021

jkmassel changed the base branch from release/16.6 to release/16.7 February 8, 2021 23:50

AliSoftware requested changes Feb 9, 2021

View reviewed changes

loremattei suggested changes Feb 10, 2021

View reviewed changes

Scripts/update-translations.rb Outdated Show resolved Hide resolved

jkmassel mentioned this pull request Feb 11, 2021

Add "set version in xcconfig" action that takes arbitrary input wordpress-mobile/release-toolkit#209

Open

jkmassel and others added 11 commits February 11, 2021 11:42

Improve curl error handling

99c53c1

Co-authored-by: Olivier Halligon <[email protected]>

Explain the really strange escape sequence

9c5e87d

Co-authored-by: Olivier Halligon <[email protected]>

Fix parens

8efddd9

Co-authored-by: Olivier Halligon <[email protected]>

Remove promo screenshots

d76a8e6

Delete files that should’ve been ignored

d9e55ae

Remove fastlane-specific .gitignore file

e2f41e5

Fix indentation weirdness

6848df3

More spacing fixes

821f074

Another indentation fix

574dbe3

Co-authored-by: Olivier Halligon <[email protected]>

Fix ruby error

911537f

Don’t delete strings

6f0d189

jkmassel requested review from AliSoftware and loremattei February 11, 2021 20:56

loremattei approved these changes Feb 12, 2021

View reviewed changes

AliSoftware approved these changes Feb 12, 2021

View reviewed changes

jkmassel merged commit e020c26 into release/16.7 Feb 12, 2021

jkmassel deleted the move/fastlane branch February 12, 2021 19:10

This was referenced Feb 12, 2021

Merge the "Move fastlane into project root" changes into develop #15884

Merged

Fix translator builds #15270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Fastlane to Project Root #15706

Move Fastlane to Project Root #15706

jkmassel commented Jan 26, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 26, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 26, 2021 •

edited

Loading

AliSoftware left a comment

loremattei left a comment

loremattei Feb 2, 2021

jkmassel Feb 3, 2021 •

edited

Loading

loremattei Feb 2, 2021

AliSoftware Feb 2, 2021 •

edited

Loading

jkmassel Feb 2, 2021

AliSoftware Feb 3, 2021

loremattei Feb 2, 2021

jkmassel Feb 2, 2021

loremattei Feb 2, 2021

jkmassel Feb 2, 2021

jkmassel Feb 3, 2021

loremattei Feb 2, 2021

jkmassel Feb 3, 2021

jkmassel commented Feb 3, 2021

AliSoftware left a comment •

edited

Loading

AliSoftware commented Feb 9, 2021 •

edited

Loading

loremattei left a comment

jkmassel commented Feb 11, 2021

loremattei left a comment

AliSoftware left a comment •

edited

Loading

		ENV["DOWNLOAD_METADATA"]="./fastlane/download_metadata.swift"
		ENV["PROJECT_ROOT_FOLDER"]="../"

Move Fastlane to Project Root #15706

Move Fastlane to Project Root #15706

Conversation

jkmassel commented Jan 26, 2021 • edited Loading

peril-wordpress-mobile bot commented Jan 26, 2021 • edited Loading

peril-wordpress-mobile bot commented Jan 26, 2021 • edited Loading

AliSoftware left a comment

Choose a reason for hiding this comment

loremattei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkmassel Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkmassel commented Feb 3, 2021

AliSoftware left a comment • edited Loading

Choose a reason for hiding this comment

AliSoftware commented Feb 9, 2021 • edited Loading

loremattei left a comment

Choose a reason for hiding this comment

jkmassel commented Feb 11, 2021

loremattei left a comment

Choose a reason for hiding this comment

AliSoftware left a comment • edited Loading

Choose a reason for hiding this comment

jkmassel commented Jan 26, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 26, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 26, 2021 •

edited

Loading

jkmassel Feb 3, 2021 •

edited

Loading

AliSoftware Feb 2, 2021 •

edited

Loading

AliSoftware left a comment •

edited

Loading

AliSoftware commented Feb 9, 2021 •

edited

Loading

AliSoftware left a comment •

edited

Loading