-
Notifications
You must be signed in to change notification settings - Fork 237
split the unsafeFlags-using manifest to a Swift 6.2 minimum version #245
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
@swift-ci Please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change itself looks fine to me. I do however have a question about the cherry-picking. Should this go into release/6.2 or should there be a different branch as 6.2.0 was released, and we don't want to alter the history of what went into that release. Similarly, I think that 0.7 has been released, and so perhaps this should go into a 0.7.1?
The |
"CMakeLists.txt" | ||
], | ||
swiftSettings: markdownSwiftSettings | ||
swiftSettings: [.unsafeFlags(["-Xcc", "-DCMARK_GFM_STATIC_DEFINE"], .when(platforms: [.windows]))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all platforms have this flag? What does it do for the platforms that didn't need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a question for @compnerd than me. I think there's a difference between how packages are linked on Windows and elsewhere that requires that we specify static
linkage for Windows when building toolchains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies. I realize now that my question sounded very different from what I meant to ask.
What I meant was: is there any impact on the other platforms of always specifying this but with a when: .windows
condition compared to only adding it inside a #if
check, like it was before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On non-Windows platforms, it normally wouldn't have any impact. However, if you were to build with -fvisibility-default=hidden
it would internalise the symbols, preventing them from participating in dynamic linking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d-ronnqvist The syntactic difference between .when(platforms: [.windows])
and #if os(Windows)
is that the latter restricts the flag to building from a Windows host, but the former restricts the flag to building for a Windows target. Switching it back to the .when
form allows us to cross-compile Swift-Markdown for Windows from other hosts (which is important for toolchain builds) with correct linkage.
Bug/issue #, if applicable: N/A
Summary
The patch to Package.swift introduced in #240 is an incomplete fix. It turns out that all platforms blocked
unsafeFlags
in dependencies, specifically prior to the release of Swift 6.2. However, this is predicated on the package'sswift-tools-version
, not just the version of SwiftPM is being used to build the package.This PR splits the package manifest into two versions:
Package.swift
now requires a tools-version of 6.2, and reinstates the use ofunsafeFlags
as of build: adjust the build on Windows to avoid some warnings #213, so that Windows builds (even builds being cross-compiled from other platforms) can correctly build Swift-Markdown and swift-cmark. This version forces the language mode to Swift 5, to prevent the strict concurrency checking from failing the build.[email protected]
manifest reinstates the older variant of the manifest, without anyunsafeFlags
, for use from older toolchains and older tools versions.I intend to cherry-pick this change onto
release/6.2
andswift-markdown-0.7
, so that a proper fix can be present in both toolchain builds and package uses.Dependencies
None
Testing
To verify the desired behavior, i have pushed a tag to my personal fork,
9999.0.0-alpha.2
, that can be used to verify the desired behavior. This can be used with the following dependency declaration:I have tested building such a package dependency with both Swift 6.2 (via Xcode 26) and Swift 6.0 (via Xcode 16), without issue.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded