Skip to content

Emit a descriptive error when running the generator on iOS, tvOS, or watchOS #87

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

Merged
merged 7 commits into from
Jun 22, 2023

Conversation

czechboy0
Copy link
Contributor

Motivation

Resolves #86, which some adopters are hitting by accident.

Modifications

Makes the generator itself build successfully on iOS, tvOS, watchOS, but not really, as I just added an explicit, clear error on those platforms pointing out that the adopter probably did this by accident, and how to fix it.

Result

A clear, descriptive error is now emitted when the adopter links the _OpenAPIGeneratorCore target from e.g. an iOS app (most likely by accident):

.../swift-openapi-generator/Sources/_OpenAPIGeneratorCore/PlatformChecks.swift:4:8 Running the generator tool itself is not supported on iOS, tvOS, and watchOS. Check that your app is not linking the generator directly. For details, check out: https://github.com/apple/swift-openapi-generator/issues/86

Test Plan

Tested locally by creating an iOS app with a lower deployment target and linked the generator directly. The error goes away when the linkages is removed, as expected.

@czechboy0 czechboy0 requested a review from simonjbeaumont June 21, 2023 19:42
@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Jun 22, 2023
Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

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

If we removed _OpenAPIGeneratorCore from the products then I think this is less likely to trip folks up. When I tested that approach locally, it is no longer offered by Xcode when adding the package dependency.

@czechboy0 czechboy0 merged commit cc8e0ad into apple:main Jun 22, 2023
@czechboy0 czechboy0 deleted the hd-86-catch-apps-linking-generator branch June 22, 2023 14:16
czechboy0 added a commit that referenced this pull request Jul 18, 2023
Lower unsupported platform minimum versions

### Motivation

Reported in #124.

In 0.1.4 (in PR #87), we improved the build time errors emitted when an adopter tries to link the generator binary itself to a product that runs on iOS, tvOS, etc (non-macOS/Linux).

Unfortunately, that broke the ability to build multi-platform packages for iOS and friends, as the minimum platform check would fail (we claim we support iOS "99"). 

Some build systems and IDEs don't correctly separate platform checks for plugins (which run on the host platform) and the final binary (which runs on the destination platform). We should help report these issues, but at the same time, we should fix this regression in 0.1.4 to work at least as well as 0.1.3 did.

### Modifications

Lower the minimum platforms to match the runtime library. This doesn't actually make the generate build correctly on these platforms (as we still have `PlatformChecks.swift`, which contains a hard, descriptive error when building on iOS and friends), it just allows passing the (incorrect) platform check in some cases.

### Result

Fixes #124.

Now, adopters can build on the same platforms they could with 0.1.3, and we still don't build on the unsupported platforms.

### Test Plan

Tested by using a local workspace and verified that with this change, a simple package that uses the generator can build for iOS.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (integration test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit a descriptive error when running the generator on iOS, tvOS, or watchOS
3 participants