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.

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

Remove explicit macOS platform requirement from Package.swift #802

Merged
merged 2 commits into from
May 9, 2024

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented May 7, 2024

It was there because of an error related to the WordPressShared dependency using the SwiftLint build plugin. I discovered what I think is a better approach to set up the dependency, which does not require explicitly declaring macOS support.

Before

image

After

image

Testing Details

See green CI.

However, notice CI doesn't build the package, so the test might give a false positive for this change.

One way to verify it is to run the following script:

#!/bin/bash

# Quit Xcode in case there is a version of WordPressKit open that could give us trouble
osascript -e 'tell application "Xcode" to quit'

mkdir -p xcode_staging
mv ./WordPressKit.xcodeproj xcode_staging
mv ./WordPressKit.xcworkspace xcode_staging

bundle install

# The device parameter is required (when running standalone?) or the underlying xcodebuild call will fail with:
# Building a Swift package requires that a destination is provided using the "-destination" option. [...]
bundle exec fastlane run run_tests \
  package_path:. \
  scheme:WordPressKit-Package \
  device:'iPhone 15 Pro'

mv xcode_staging/* .

rm -rf xcode_staging

  • Please check here if your pull request includes additional test coverage. — N.A.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary. — N.A.

SPM fails in this commit, we'll fix it in the next.
Leaving it in this state for demonstration purposes.
This prevents builds failing with:

> error: the library 'CoreAPI' requires macos 10.13, but depends on the
> product 'WordPressShared' which requires macos 12.0; consider changing
> the library 'CoreAPI' to require macos 12.0 or later, or the product
> 'WordPressShared' to require macos 10.13 or earlier.

WordPressShared internally depends on the SwiftLint plugin, which
explicitly declares a macOS version.
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ Package.swift was changed without updating its corresponding Package.resolved. Please resolve the Swift packages in Xcode.

Generated by 🚫 Danger

@iangmaia
Copy link
Contributor

iangmaia commented May 7, 2024

@mokagio about the @dangermattic warning below, I guess it does make sense on this case? Or there was no change in the Package.resolved?

1 Warning
⚠️ Package.swift was changed without updating its corresponding Package.resolved. Please resolve the Swift packages in Xcode.
Generated by 🚫 Danger

@mokagio
Copy link
Contributor Author

mokagio commented May 8, 2024

@iangmaia good observation, thanks.

Package.swift changed, so Danger did it's job posting the warning. However, the warning's wording is misleading because the kind of change we had here does not affect the Package.resolved content.

I think the wording is an adaptation of what we used for Podfile and Podfile.lock. The difference is that in CocoaPods every change to Podfile results to a Podfile.lock change because the lockfile contains a hash of the Podfile. But SPM does not do that.

Maybe we should update the wording... 🤔

⚠️ Package.swift was changed but its corresponding Package.resolved was not. Please ensure Package.resolved is up to date if any of the dependencies in Package.swift changed.

Alternatively, we could improve the logic that picks up the Package.swift changes. But that might prove tricky given that's a Swift file which can contain code etc. I remember @crazytonyli wrote some SourceKit code to compute certain changes, so maybe it could be done. But... I wonder about the ROI of going down this particular rabbit hole.

@iangmaia
Copy link
Contributor

iangmaia commented May 9, 2024

However, the warning's wording is misleading because the kind of change we had here does not affect the Package.resolved content.

That's what I was unsure about, though I was imagining Package.resolved changes would be needed.

Maybe we should update the wording... 🤔

⚠️ Package.swift was changed but its corresponding Package.resolved was not. Please ensure Package.resolved is up to date if any of the dependencies in Package.swift changed.

That's better indeed!

Alternatively, we could improve the logic that picks up the Package.swift changes. [...] I wonder about the ROI of going down this particular rabbit hole.

Indeed! I think an improved message already helps a bit. I've then created the issue Automattic/dangermattic#65

@mokagio mokagio merged commit 30e9472 into trunk May 9, 2024
9 checks passed
@mokagio mokagio deleted the mokagio/remove-macos-platform-from-package branch May 9, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants