Skip to content

Verify product dependencies w/ target platform, vs. hardcoded macOS #6963

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

Conversation

peterkos
Copy link
Contributor

@peterkos peterkos commented Oct 1, 2023

Product dependencies are now verified using the buildEnvironment's platform, instead of the hardcoded macOS platform.

This builds on the work of #6732, now allowing cross-compilation of product dependencies 🎉

Motivation:

Let's say we're cross-compiling macOS -> iOS simulator, with a host system of macOS v13.

let package = Package(
    name: "SwiftFlashlight",
    products: [.library(...)],
    platforms: [.iOS(.v16)],
    dependencies: [
        // internally, for iOS(.v16), macOS(.v14)
        .package(url: "probably/alamofire", exact: "1.2.3")
    ],
    // ...
)

As long as we set our triple & SDK, we should be good... (and, with #6828, only the triple needed)

> swift build --triple arm64-apple-ios-simulator \
              --sdk "$(xcrun --sdk iphonesimulator --show-sdk-path)"
...
error: the library 'SwiftFlashlight' requires macos 10.13,
but depends on the product 'probably/alamofire' which requires macos 10.14;  [...]

Even though SwiftFlashlight only supports iOS(.v16), SupportedPlatform "[by] default, [...] assigns a predefined minimum deployment version for each supported platforms unless you configure supported platforms [...]". As a side-effect, macOS is inferred as a valid platform. (This is why no "target/platform mismatch" error was thrown.)

Previously, product dependencies were verified against macOS, rather than the target platform. Since macOS is always inferred, this surfaced as a "woah! your dependencies are out of sync".

let swiftFlashlight = Package(
    platforms: [
        .iOS(.v16),
        .macOS(.v13) // <-- inferred-|
    ],                               |
)                                    |-- // since --triple is .iOS,
                                     |   // shouldn't be comparing
let probably/alamofire = Package (   |   // macOS.
    platforms: [                     |
        iOS(.v16),                   |
        macOS(.v14)] // <-- inferred-|
    ]
)

Now, the intention of --triple is respected 🐱

Modifications:

  • validateDeploymentVersionOfProductDependency accepts a buildEnvironment containing the target platform

Result:

Product dependencies can now be compiled for darwin other than macOS -- i.e, iOS :)

Future work

  • Target-level or product-level platform support is still up in the air
  • This only applies to triple.isDarwin() -- perhaps a next step of, verifying dependencies against non-darwin?

@peterkos
Copy link
Contributor Author

peterkos commented Oct 1, 2023

This was a fun one to chase down 😅
(note to self: Apple Swift version 5.9 (swift-5.9-RELEASE))

Some questions:

  • I tried running swiftformat but it changed 600+ files -- just BuildPlan was ~300 lines 😵, thoughts?
  • Happy to add some tests around this; wanted to get some thoughts before diving in there :)

@MaxDesiatov
Copy link
Contributor

I tried running swiftformat but it changed 600+ files -- just BuildPlan was ~300 lines 😵, thoughts?

Please run Utilities/soundness.sh script, which will only format files that were changed.

Happy to add some tests around this; wanted to get some thoughts before diving in there :)

Yes, please add the tests, that would be great. Thanks!

@peterkos
Copy link
Contributor Author

peterkos commented Oct 2, 2023

Thanks! I'll take a crack at it soon :)

@MaxDesiatov MaxDesiatov added the needs tests This change needs test coverage label Oct 10, 2023
…latform

# Conflicts:
#	Sources/Build/BuildPlan.swift
Cherry-picked given OG file was moved into `BuildPlan/` folder
- Tweaked existing validation test to ensure no overlap w/ this new test
- Added `Triple.arm64iOS` for conveinence
Manually triggered by adding newline (which formatter would auto-remove) to help make PR diff a bit cleaner :)
@peterkos
Copy link
Contributor Author

peterkos commented Dec 10, 2023

@MaxDesiatov Apologies for the delay on this! One test added, one test modified 🧪

Since utilities/soundness.sh works from the git diff, I added a newline in each file I modified + ran the script as a separate commit for #reviewsanity. As you can see, those files haven't had a run in a while, heh.

@MaxDesiatov
Copy link
Contributor

Thanks! Just as heads up, this will conflict with #7161

@MaxDesiatov MaxDesiatov self-assigned this Dec 11, 2023
…into peterkos/validate-product-deps-against-env-platform

# Conflicts:
#	Sources/Build/BuildPlan/BuildPlan.swift
@peterkos
Copy link
Contributor Author

peterkos commented Dec 27, 2023

Resolved conflicts 👍

Looks like soundness.sh disagrees with how Swift requires raw string literals to be formatted tho. One example:

// BuildPlanTests L#5749
func testArtifactsArchiveBinaryTargets(...) {
    // ...
                        "path": "all-platforms/mytool",
                        "supportedTriples": ["\(
                            artifactTriples.map(\.tripleString)
                                .joined(separator: "\", \"")
    )"]
                }
            ]
        }
    }
}
                    """
                //   ^ swiftformat throws this here,
                //     but is a swift build error :(

I've manually reverted the script's raw string changes for now, but it seems like the way swiftformat expands inline closures (among other things) could be improved -- that's my guess 🚀

Also the soundness.sh script didn't print any error output unless I tweaked L60. Assuming it's a me-issue, but can push up that change if useful.

swiftformat $changed_file > dev/null 2>&1 # old
swiftformat $changed_file 2>&1            # new

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor

@swift-ci test

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thank you!

@MaxDesiatov MaxDesiatov removed the needs tests This change needs test coverage label Jan 2, 2024
@MaxDesiatov MaxDesiatov changed the title Verify product dependencies using the build env platform, vs. hardcoded macOS Verify product dependencies w/ target platform, vs. hardcoded macOS Jan 2, 2024
@MaxDesiatov MaxDesiatov enabled auto-merge (squash) January 2, 2024 12:14
@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit ac1680d into swiftlang:main Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants