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

Relax dependency on SwiftSyntax and SwiftFormat to 508..<510 #331

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

mbrandonw
Copy link
Contributor

Motivation

Currently this project pins to a specific major version of SwiftSyntax and SwiftFormat, which is 508. That means anyone depending on this project cannot also use macros, because that typically requires depending on SwiftSyntax 509.

Modifications

I relaxed the dependency constraint to be "508.0.1"..<"510.0.0".

Result

There is only one small change that needs to be done to support SwiftSyntax 509, and it can easily be worked around by checking #if canImport(SwiftSyntax509).

Test Plan

This change does complicate testing a bit since there are subtle formatting changes in SwiftFormat 508 vs 509. In particular, the Client.swift file has a few lines that get formatted onto 2 lines, whereas previously they were allowed to be on a single line.

This means the test suite will pass when SwiftFormat 508 is used but fail when 509 is used. So, for now, I've decided to just omit the test when 509 is available, but maybe there is some way to support both Swift versions in the test. I am not very familiar with this library or how it's tests are set up.

@czechboy0
Copy link
Contributor

Hi @mbrandonw - thanks so much! Regarding the tests, let's keep with the times and make the tests pass with 5.9 instead, as that's what happens for a new project. If someone gets 5.8 resolved, that's okay, we know it's worked till now.

Otherwise looks great!

@mbrandonw
Copy link
Contributor Author

@czechboy0 No problem, pushed fixes to the tests for SwiftFormat 509.

@czechboy0
Copy link
Contributor

@swift-server-bot add to allowlist

Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks! Please just check the CI failures, we'll need to get them to green before landing.

@mbrandonw
Copy link
Contributor Author

Hi @czechboy0, I don't know why the build is failing. The Jenkins logs show that all tests pass, yet it then says:

Build step 'Execute shell' marked build as failure

Maybe someone more familiar with y'alls CI should take this over. I don't think there's much I can do to help.

@czechboy0
Copy link
Contributor

I can take a look Monday, but first thing I search for is "error:".

@mbrandonw
Copy link
Contributor Author

error: Target SwiftSyntaxMacroExpansion imports another target (SwiftOperators) in the package without declaring it a dependency.

Looks like something for SwiftSyntax to address, or for OpenAPIGenerator to ignore:

https://github.com/apple/swift-syntax/blob/74203046135342e4a4a627476dd6caf8b28fe11b/Package.swift#L190-L194

@czechboy0
Copy link
Contributor

Okay, unfortunately it seems that swift-syntax has some missing dependencies, filed here: swiftlang/swift-syntax#2287

To make this PR pass, can you comment out the line - IMPORT_CHECK_ARG=--explicit-target-dependency-import-check error in all 4 docker-compose files in the docker directory? I filed another issue for us to re-enable it later: #332

@mbrandonw
Copy link
Contributor Author

Looks like it's now passing. 🙂

@czechboy0 czechboy0 merged commit d9d5daa into apple:main Oct 16, 2023
@czechboy0
Copy link
Contributor

Thank you @mbrandonw and @groue!

@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Oct 19, 2023
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.

2 participants