Add SPM Caching Action #43

spencertransier · 2023-03-01T22:42:14Z

Summary

This PR adds a caching action for Swift Package Manager to the CI toolkit. Some notes:

It works for both workspace-based repos like WCiOS, DOiOS/Mac, etc. along with non-Xcode repos like hostmgr or Swift packages if they have SPM dependencies. The action automatically detects what kind of repo it is based on where the Package.resolved file is located.
I used the existing install_swiftpm_dependencies action because I didn't see it being used elsewhere. If that's incorrect (@jkmassel & @AliSoftware looks like you both worked on that action in the past), please let me know and I can move SPM caching into a separate action.
This is my first time diving into bash scripting. If there are conventions I'm not following or better ways of doing something, please let me know!

How to Test

You can see this CI build of Day One iOS/Mac that's using the latest caching code:

Example Builds:

DOiOS/Mac: https://buildkite.com/automattic/day-one-apple/builds/7705
WCiOS: https://buildkite.com/automattic/woocommerce-ios/builds/11347
hostmgr: https://buildkite.com/automattic/hostmgr/builds/295

There is now a "Restoring SPM Cache" step:

To try regenerating the cache, you can delete any of the day-one-spm-cache... files from the a8c-ci-cache and re-run the build to verify that the cache rebuilds.

This reverts commit 5fb1859.

spencertransier · 2023-03-01T23:51:45Z

Question about the Shellcheck error I'm getting:

In bin/save_cache line 58:
[2023-03-01T23:35:12Z] tar $TAR_OPTIONS -czf "$CACHE_KEY" -C "$CACHE_FILE" .
[2023-03-01T23:35:12Z] ^----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

It's referring to $TAR_OPTIONS not being surrounded in quotes. The issue is that if I add the quotes, the tar options that I'm passing to save_cache from install_swiftpm_dependencies don't actually get used by tar when archiving the cache. In this case, I'm passing folders that should be excluded when creating the cache archive.

Without the quotes around $TAR_OPTIONS in save_cache, the folders are excluded from the archive as expected. With the quotes, the --exclude options are ignored and the folders are not excluded from the archive.

I'm just not sure why adding the quotes is doing that. Any suggestions about what could be changed to allow for the quotes to be added? I know this is just an info issue with Shellcheck. I assume we have Shellcheck fail for info issues for a reason, so I didn't want to ignore this one even though it's not even a warning.

AliSoftware · 2023-03-02T00:44:57Z

@spencertransier Adding the quotes around "$TAR_OPTIONS" means that if that variable contains eg --exclude Foo then it will consider "--exclude foo" as a single argument (one which contains spaces) as opposed to two.

On the other hand, not quoting the $TAR_OPTIONS means that if it contains --exclude foo it'll see it as two individual arguments like you probably want… but that also means that if the folder you want to exclude contains a space, eg --exclude my project it will see it as three distinct arguments (--exclude, my and project)

The best solution for your case is probably to use the bash array syntax to build your list or tar options (whose syntax I always forget but a quick search on the net should help)

crazytonyli · 2023-03-02T01:59:00Z

bin/install_swiftpm_dependencies

+# `artifacts` should be excluded because it causes issues when downloading 
+# certain packages to have the artifacts already present after extracting 
+# cache
+TAR_OPTIONS="--exclude=./checkouts --exclude=./artifacts"


Rather than fighting with bash syntax regarding using quote or not, I'm wondering if it'd be simpler to just delete these unwanted files from the SPM_CACHE_LOCATION?

Or, copy the "SPM_CACHE_LOCATION", delete the unwanted files from the new copy, call save_cache on the new copy, and delete the new copy afterwards.

@AliSoftware Thank you for the explanation! The array solution makes a lot of sense after the behavior I saw while debugging this.

@crazytonyli Good idea! I think I'm going to go with deleting those directories before saving the cache. It removes the complexity of passing the tar options along with simplifying the changes to save_cache.

bin/save_cache

bin/install_swiftpm_dependencies

AliSoftware · 2023-03-02T11:29:15Z

bin/install_swiftpm_dependencies


+CUSTOM_PACKAGE_RESOLVED_PATH=${1-}
+BUILD_TYPE=${2-}
+SPM_CACHE_LOCATION="$HOME/spm"


There exists a $HOME/.swiftpm/cache folder on my Mac that was probably created by SPM itself at some point.

I wonder if we shouldn't use that folder (for conventions) as opposed to ~/spm here?

Ah, good point! Jeremy and I had discussed using the cache that's within DerivedData, but I wasn't able to make that work because there's not a consistent path name within DerivedData. But it does work great with making the cache location in the script point to $HOME/Library/Caches/org.swift.swiftpm. And that has the added benefit of client repos not needing to specify a specific SPM cache folder in the Fastfile. Package resolution will pull from that org.swift.swiftpm folder automatically when resolving the packages.

Added in 44785e4

bin/install_swiftpm_dependencies

AliSoftware · 2023-03-02T11:42:06Z

bin/save_cache

 	echo "	Compressing"
-	tar -czf "$CACHE_KEY" "$CACHE_FILE"
+	TAR_CONFIG=${4-}
+	if [[ "$TAR_CONFIG" == '--use_alternate_tar_config' ]]; then


Nit but I'm not a fan of the name of this custom flag… what about --use_relative_path_in_tar or something like that instead?

Also, I'm wondering if it wouldn't make sense to adopt that -C behavior… in all circumstances after all (seems a nicer solution than including /User/builder in the cache path and risking the un-tar-ing to fail if used on a different builder which might not have a /User/builder folder (all our Mac VMs currently do, but we shouldn't need to make that assumption, and also nothing says the cache would be restored on a Mac VM vs an Android AMI or Docker container or whatnot, so…).

That one might be a broader investigation to verify that changing that behavior that save_cache was using so far (and you kept in the else) to instead use -C "$CACHE_FILE" . in all cases would not break existing setups depending on how they use restore_cache on their end; so could be saved for a subsequent PR if you're not sure about that. But would be a nice simplification if we can confirm it and go in that direction.

Agreed, I like --use_relative_path_in_tar better, too. Changed in b919255

I'm wondering if it wouldn't make sense to adopt that -C behavior in all circumstances.

I agree that would be nice to use the same behavior for all cases if possible. Because I saw different behavior between the existing tar configuration versus the one I added in this PR, I had assumed the projects that use save_cache would need to be updated and cause a breaking change. I haven't tested that out yet, but I think it'd be outside the scope for this project. I'll create a separate issue for it.

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

crazytonyli · 2023-03-06T01:39:57Z

bin/install_swiftpm_dependencies

+
+# If this is the first time we've seen this particular cache key, save it for the future
+echo "	Saving SPM Cache"
+save_cache "$SPM_CACHE_LOCATION" "$CACHE_KEY" false --use_relative_path_in_tar


Not a blocker. This sequence of argument feels prone to errors, it's probably worth looking into parsing the arguments and options, using getopts for example. Statement as below would look much nicer 😸

save_cache --cache-key "$CACHE_KEY" --overwrite --use_relative_path_in_tar "$SPM_CACHE_LOCATION"

Thank you for the suggestion! I am going to land this PR and work on improving this in my next iteration of the SPM caching and save_cache improvements.

bin/install_swiftpm_dependencies

crazytonyli · 2023-03-06T01:45:46Z

bin/install_swiftpm_dependencies

+cd -
+
+# This will let Xcode use the system SSH config for downloading packages
 sudo defaults write com.apple.dt.Xcode IDEPackageSupportUseBuiltinSCM YES


Not part of this PR. Is this preference same as xcodebuild -scmProvider system?

@crazytonyli I think so but I'm not sure. I will look into this and see about adding in the next update of the SPM caching action.

spencertransier added 30 commits February 8, 2023 15:45

Create restore_spm_cache

4fe69cf

Add separate save_spm_cache action

702c16e

Try different path to get relative path

5fb1859

Revert "Try different path to get relative path"

bf61231

This reverts commit 5fb1859.

Remove --quiet from aws commands

0697fb3

Test different tar option

ca48379

Restore cache to correct location

1f19980

Move the commands over to install_swiftpm_dependencies

dc1ef56

Fix linter issue

9c1565a

Automatically find Package.resolved

4855a65

Package.resolved variable isn't initialized yet

ac1697b

Exclude 'checkouts' and 'artifacts' from SPM cache

776efe7

Switch back to root project directory after restoring cache

e6813ff

Test with explicit workspace path

74e046b

Change location of SPM cache

7237304

Change where to save cache from

cdd1414

Install the SPM packages in the correct location

38c1a99

Use swift or xcodebuild depending on the project type

543042e

Use Apple Archive for archiving and extracting the cache

8bfe4f7

Update install_swiftpm_dependencies

68b361b

Add lines to verify archive/extract method

b3fe073

Re-add --quiet to AWS upload

d988855

Add custom path

1423e71

Collapse aa archive into one line

9432ed6

Use uname to determine whether to use Apple Archive

71f18ab

Continue iterating on Apple Archive support

f71842b

Remove Apple Archive option

f3ba705

Use $HOME instead of ~

f08a599

Use the correct TAR_OPTIONS

1b518d5

Cleaned up debugging echos and added explanatory comments

bdaf289

spencertransier added 2 commits March 1, 2023 16:24

Fix shellcheck warnings

48bc2ac

Fix shellcheck issue - SC2128

f8942a8

spencertransier marked this pull request as ready for review March 2, 2023 00:08

spencertransier requested review from a team and jkmassel March 2, 2023 00:08

crazytonyli reviewed Mar 2, 2023

View reviewed changes

AliSoftware reviewed Mar 2, 2023

View reviewed changes

bin/save_cache Outdated Show resolved Hide resolved

spencertransier added 2 commits March 1, 2023 19:52

Simplify saving cache by deleting unneeded cache files beforehand

8a6377f

Fix indenting

e8784f5

spencertransier requested a review from AliSoftware March 2, 2023 05:46

AliSoftware reviewed Mar 2, 2023

View reviewed changes

spencertransier and others added 4 commits March 2, 2023 08:33

Add curly braces around CACHE_KEY variables

c05d5e8

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

Add quotes around all variable names in if statement

82af9b6

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

Use default SPM location

44785e4

Update tar custom flag name

b919255

spencertransier mentioned this pull request Mar 2, 2023

Research using relative path everywhere for tar in save_cache #45

Open

spencertransier requested a review from jkmassel March 3, 2023 22:08

jkmassel approved these changes Mar 3, 2023

View reviewed changes

spencertransier requested review from AliSoftware and crazytonyli March 3, 2023 22:08

crazytonyli reviewed Mar 6, 2023

View reviewed changes

crazytonyli requested changes Mar 6, 2023

View reviewed changes

Exit if unable to find Package.resolved or if BUILD_TYPE is not set

9d49421

crazytonyli approved these changes Mar 9, 2023

View reviewed changes

Merge branch 'trunk' into add/spm-caching-actions

4cec047

spencertransier enabled auto-merge (squash) March 9, 2023 22:45

spencertransier merged commit e257e89 into trunk Mar 9, 2023

spencertransier deleted the add/spm-caching-actions branch March 9, 2023 22:46

Add SPM Caching Action #43

Add SPM Caching Action #43

Uh oh!

Conversation

spencertransier commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How to Test

Uh oh!

spencertransier commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spencertransier commented Mar 1, 2023 •

edited

Loading

spencertransier commented Mar 1, 2023 •

edited

Loading

AliSoftware commented Mar 2, 2023 •

edited

Loading