Optionaly fail install_swiftpm_dependencies when Package.resolved is not current

[andrewdmontgomery]

Add an optional flag (--strict-package-check) that will cause the plugin to exit non-zero if the Package.resolved is not current.

Update the print_usage() function output to handle the new optional flag better.

Since the argument parsing requires looping, add logic to prevent multiple target types from being passed.

• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[dangermattic]

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

[andrewdmontgomery]

@AliSoftware, do you think this is an appropriate place for this functionality? Would we rather have a dedicated plugin that fails if certain paths have uncommitted changes? I think this "uncommitted changes" check should target specific paths, since there could be other uncommitted changes that shouldn't cause this to fail. So this seems like a good place to enforce this.

I love the tests. Adding this optional flag will complicate the testing. If we think this is a good way to solve this need, I can:

1. create duplicate of tests/install_swiftpm_dependencies/package with an out-of-date Package.Resolved
2. same with project and workspace
3. create new test_* scripts for each scenario
4. update the pipeline accordingly

[andrewdmontgomery]

I'm not entirely sure that this functions equivalently for existing use cases. Here's the problem:

1. Remove dependency from Package.swift
2. Do not update Package.resolved

If -onlyUsePackageVersionsFromResolvedFile is used, the Package.resolved does not change. That's ok for existing use cases for this toolkit plugin. But for our use case, we need to know that the Package.resolved is out of date.

The descriptions of these flags in xcodebuild -h are not specific enough to help.

[andrewdmontgomery]

Changing this flag for this case seems dicey to me. And now that I know about the Dangerfile plugin to check this (thank you), this PR seems like a risky place to handle this.

[AliSoftware]

The use of -skipPackageUpdates is not the same as -onlyUsePackageVersionsFromResolvedFile (which is an equivalent to the legacy -disableAutomaticPackageResolution FYI).

According to the xcodebuild -help page:

  -resolvePackageDependencies                              resolves any Swift package dependencies referenced by the project or workspace
  -disableAutomaticPackageResolution                       prevents packages from automatically being resolved to versions other than those recorded in the `Package.resolved` file
  -onlyUsePackageVersionsFromResolvedFile                  prevents packages from automatically being resolved to versions other than those recorded in the `Package.resolved` file
  -skipPackageUpdates                                      Skip updating package dependencies from their remote

One of the important aspect of this helper running on our CI builds is that we don't want CI to risk updating any of the local packages used by the build, and instead want to ensure that it will only install the exact same set of packages, with the exact version being locked in the .resolved file, on CI every time, for reproducibility and idempotence of CI builds, which is a pretty important attribute to have on our CI.

In a way, xcodebuild -onlyUsePackageVersionsFromResolvedFile is to pod install what xcodebuild -resolvePackageDependencies is to pod update (and Package.resolved is SPM's pendant to Podfile.lock).
The former ensures that we install just exactly the versions that are listed in the lockfile and guarantee that we won't update any dependency while doing so but instead install the exact same set of dependencies on all other devs and machines building that commit. After all that's the whole point of having a lockfile, it's to lock in place the versions of the dependencies that should be installed, regardless on which machines they are installed on and when they are being installed (i.e. even if new versions of some deps have been released since you first ran the command). The latter is quite the opposite, as it allows you to disregard the versions that have been locked in the lockfile (Package.resolved) and allow to resolve to newer versions of those dependencies if any are available. Which is great for keeping things up-to-date with all the latest bugfixes and all, but is contrary to the idempotence and reproducibility key attributes important to us in a CI environment.

---

So TL;DR: I'd advice keeping -onlyUsePackageVersionsFromResolvedFile instead of switching to use -skipPackageUpdates which might still risk updating the dependencies under you.

Using -onlyUsePackageVersionsFromResolvedFile also implies that Package.resolved should not end up being modified by that command, and so your git status --porcelain check below would not make sense in such CI environment.

[AliSoftware]

As discussed in this other PR the addition of --strict-package-check to this helper might not be that necessary after all given that the Danger plugin should already prevent us from forgetting about updating the Package.resolved

Additionally, as discussed in the comment above, we want to ensure CI idempotence of builds done on the same commit, so we want to use -onlyUsePackageVersionsFromResolvedFile to guarantee Package.resolved would never risk being modified by CI itself during a build.

---

This means that this PR is mostly obsolete after all, at least wrt to its original purpose of adding the optional check about Package.resolved not being current.

That being said, I like the improvements you provided to the print_usage()—reads much more clearly 👍 —so maybe we should still keep that part from this PR, even if you revert all the rest? Repurposing this PR into one that just improves the documentation of the helper instead.

[AliSoftware]

Suggested change

	4. Default behavior (Attept to find an appropriate workspace, project, or swift package):
	4. Default behavior (Attempt to find an appropriate workspace, project, or swift package automatically):

[AliSoftware]

Seems this variable is now completely unused (I'm guessing it was initially introduced as an attempt to detect argument conflicts but then you switched to test for if [[ -n $XCWORKSPACE_PATH || -n $XCODEPROJ_PATH || $USE_SPM == "true" ]] in each branch since?

[AliSoftware]

I wonder if we couldn't DRY this check in a dedicated local function, e.g.

function check_options_conflicts() {
  if [[ -n $XCWORKSPACE_PATH || -n $XCODEPROJ_PATH || $USE_SPM == "true" ]]; then
    echo "Error: only one mode can be used: --workspace, --project, or --use-spm"
    exit 1
  fi
}

Then you'd simply call check_options_conflicts() in each of the case instead of repeating those lines.

I also changed the wording from "target" to "mode" as I felt that the word "target" was confusing (I initially thought you meant it as in "Xcode targets" and xcodebuild's --target parameter)

---

That being said. since we'll end up removing the --strict-package-check option your PR initially added here (see other comments), this means we'll go back to not needing the while loop to wrap the case ${1:-} block… and thus this additional check for conflicting/duplicate options will not be necessary in the first place. And we already check for unexpected extra arguments on line L37 / R88 after the case block, so once you revert those changes and get rid of the added while, the script should still already detect unexpected (or duplicate) arguments.

[AliSoftware]

The use of -skipPackageUpdates is not the same as -onlyUsePackageVersionsFromResolvedFile (which is an equivalent to the legacy -disableAutomaticPackageResolution FYI).

According to the xcodebuild -help page:

  -resolvePackageDependencies                              resolves any Swift package dependencies referenced by the project or workspace
  -disableAutomaticPackageResolution                       prevents packages from automatically being resolved to versions other than those recorded in the `Package.resolved` file
  -onlyUsePackageVersionsFromResolvedFile                  prevents packages from automatically being resolved to versions other than those recorded in the `Package.resolved` file
  -skipPackageUpdates                                      Skip updating package dependencies from their remote

One of the important aspect of this helper running on our CI builds is that we don't want CI to risk updating any of the local packages used by the build, and instead want to ensure that it will only install the exact same set of packages, with the exact version being locked in the .resolved file, on CI every time, for reproducibility and idempotence of CI builds, which is a pretty important attribute to have on our CI.

In a way, xcodebuild -onlyUsePackageVersionsFromResolvedFile is to pod install what xcodebuild -resolvePackageDependencies is to pod update (and Package.resolved is SPM's pendant to Podfile.lock).
The former ensures that we install just exactly the versions that are listed in the lockfile and guarantee that we won't update any dependency while doing so but instead install the exact same set of dependencies on all other devs and machines building that commit. After all that's the whole point of having a lockfile, it's to lock in place the versions of the dependencies that should be installed, regardless on which machines they are installed on and when they are being installed (i.e. even if new versions of some deps have been released since you first ran the command). The latter is quite the opposite, as it allows you to disregard the versions that have been locked in the lockfile (Package.resolved) and allow to resolve to newer versions of those dependencies if any are available. Which is great for keeping things up-to-date with all the latest bugfixes and all, but is contrary to the idempotence and reproducibility key attributes important to us in a CI environment.

---

So TL;DR: I'd advice keeping -onlyUsePackageVersionsFromResolvedFile instead of switching to use -skipPackageUpdates which might still risk updating the dependencies under you.

Using -onlyUsePackageVersionsFromResolvedFile also implies that Package.resolved should not end up being modified by that command, and so your git status --porcelain check below would not make sense in such CI environment.

[AliSoftware]

As discussed above, this change should be reverted

[AliSoftware]

Given the discussion above that we want idempotence on CI, using -onlyUsePackageVersionsFromResolvedFile already guarantees that the Package.resolved will not be modified by CI, so this new code won't work in this context as we'll keep using -onlyUsePackageVersionsFromResolvedFile, and should thus be deleted.