Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionaly fail install_swiftpm_dependencies when Package.resolved is not current #141

Draft
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Draft

Optionaly fail install_swiftpm_dependencies when Package.resolved is not current #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eef5afe
Add support for strictly enforcing that Package.resolved is current
andrewdmontgomery Jan 21, 2025
b05c9bc
Update the `print_usage()` function to cover the new optional flag
andrewdmontgomery Jan 21, 2025
6018d40
Enforce only one target is used
andrewdmontgomery Jan 21, 2025
89390a5
Update CHANGELOG.md
andrewdmontgomery Jan 21, 2025
6650ecc
Remove logic that requires one (and only one) target to be passed
andrewdmontgomery Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ _None._

### New Features

_None._
- `install_swiftpm_dependencies`: Add optional flag to exit non-zero if the `Package.resolved` is not current [#141]

### Bug Fixes

Expand Down
118 changes: 90 additions & 28 deletions bin/install_swiftpm_dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,89 @@
#!/bin/bash -eu

print_usage() {
echo "Usage:"
echo " Using the \`Package.resolved\` of an Xcode \`.xworkspace\`:"
echo " ${0##*/} --workspace PATH"
echo " Using the \`Package.resolved\` of an Xcode \`.xcodeproj\`:"
echo " ${0##*/} --project PATH"
echo " Using the \`Package.resolved\` at the root, managed by \`swift package\` instead of Xcode:"
echo " ${0##*/} --use-spm"
cat <<EOF
Usage:
Install SwiftPM dependencies using the \`Package.resolved\` file:

1. For an Xcode \`.xcworkspace\`:
${0##*/} --workspace PATH [--strict-package-check]

2. For an Xcode \`.xcodeproj\`:
${0##*/} --project PATH [--strict-package-check]

3. For a Swift package managed by \`swift package\` instead of Xcode:
${0##*/} --use-spm [--strict-package-check]

4. Default behavior (Attept to find an appropriate workspace, project, or swift package):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

${0##*/} [--strict-package-check]

Options:
--workspace PATH Specify the path to the Xcode workspace.
--project PATH Specify the path to the Xcode project.
--use-spm Use the Swift package manager at the root.
--strict-package-check Fail the installation if the \`Package.resolved\` file has uncommitted changes or is out of date.
If specified more than once, the last value will be used.

Examples:
Install dependencies for an Xcode workspace:
${0##*/} --workspace MyApp.xcworkspace

Install dependencies with strict package check:
${0##*/} --project MyApp.xcodeproj --strict-package-check

Install dependencies for a Swift package at the root:
${0##*/} --use-spm
EOF
}

# Parse input parameters
XCWORKSPACE_PATH=
XCODEPROJ_PATH=
USE_SPM=false

case ${1:-} in
--workspace)
XCWORKSPACE_PATH="${2:?you need to provide a value after the --workspace option}"
shift; shift
;;
--project)
XCODEPROJ_PATH=${2:?you need to provide a value after the --project option}
shift; shift
;;
--use-spm)
USE_SPM=true
shift
;;
-h|--help)
print_usage
exit 0
;;
esac
STRICT_PACKAGE_CHECK=false

# Track the number of targets found in the arguments
TARGET_COUNT=0
Comment on lines +45 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


while [[ $# -gt 0 ]]; do
case ${1:-} in
--workspace)
if [[ -n $XCWORKSPACE_PATH || -n $XCODEPROJ_PATH || $USE_SPM == "true" ]]; then
echo "Error: only one target can be used: --workspace, --project, or --use-spm"
exit 1
fi
Comment on lines +51 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

XCWORKSPACE_PATH="${2:?you need to provide a value after the --workspace option}"
TARGET_COUNT=$((TARGET_COUNT + 1))
shift; shift
;;
--project)
if [[ -n $XCWORKSPACE_PATH || -n $XCODEPROJ_PATH || $USE_SPM == "true" ]]; then
echo "Error: only one target can be used: --workspace, --project, or --use-spm"
exit 1
fi
XCODEPROJ_PATH=${2:?you need to provide a value after the --project option}
TARGET_COUNT=$((TARGET_COUNT + 1))
shift; shift
;;
--use-spm)
if [[ -n $XCWORKSPACE_PATH || -n $XCODEPROJ_PATH || $USE_SPM == "true" ]]; then
echo "Error: only one target can be used: --workspace, --project, or --use-spm"
exit 1
fi
USE_SPM=true
TARGET_COUNT=$((TARGET_COUNT + 1))
shift
;;
--strict-package-check)
STRICT_PACKAGE_CHECK="true"
shift
;;
-h|--help)
print_usage
exit 0
;;
esac
done

if [[ "$#" -gt 0 ]]; then
echo "Unexpected extra arguments: $*" >&2
Expand Down Expand Up @@ -109,16 +160,27 @@ if [[ -d "${XCWORKSPACE_PATH}" ]]; then
# (despite the help page of `xcodebuild` suggesting that it should work without `-scheme`). Since the dependency resolution doesn't really depend on the scheme
# and we don't want to have to provide or guess it, using `-list` instead stops making `xcodebuild` complain about `-workspace` not being used in conjunction
# with `-scheme` (even if in practice we don't care about the scheme list it returns)
xcodebuild -workspace "${XCWORKSPACE_PATH}" -resolvePackageDependencies -onlyUsePackageVersionsFromResolvedFile -list
xcodebuild -workspace "${XCWORKSPACE_PATH}" -resolvePackageDependencies -skipPackageUpdates -list
Copy link
Author

@andrewdmontgomery andrewdmontgomery Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

elif [[ -d "${XCODEPROJ_PATH}" ]]; then
echo "~~~ Resolving Swift Packages with \`xcodebuild\`"
echo "Using -project \"${XCODEPROJ_PATH}\""
xcodebuild -project "${XCODEPROJ_PATH}" -resolvePackageDependencies -onlyUsePackageVersionsFromResolvedFile
xcodebuild -project "${XCODEPROJ_PATH}" -resolvePackageDependencies -skipPackageUpdates
Comment on lines -116 to +167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed above, this change should be reverted

elif [[ "${USE_SPM}" == "true" ]]; then
echo "~~~ Resolving packages with \`swift package\`"
swift package resolve
fi

# If STRICT_PACKAGE_CHECK is enabled, check to see if the `Package.resolved` has uncommitted changes
if [[ "$STRICT_PACKAGE_CHECK" == "true" ]]; then
CHANGES=$(git status --porcelain -- "$PACKAGE_RESOLVED_LOCATION")

if [[ -n "$CHANGES" ]]; then
echo "Uncommitted changes detected in $PACKAGE_RESOLVED_LOCATION:"
echo "$CHANGES"
exit 1
fi
fi

Comment on lines +173 to +183
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# `checkouts` can be removed because the system can quickly generate them
# instead of needing to download them in the cache each time.
#
Expand Down