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

Add support for building on Windows using SwiftPM #930

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Conversation

jakepetroules
Copy link
Contributor

This adds a dependency on the new swift-toolchain-sqlite package, which allows llbuild to build using swift build on Windows (including tests) without any additional flags or preinstalled dependencies.

rdar://133338086

@@ -213,6 +221,9 @@ let package = Package(
.headerSearchPath(".."),
.headerSearchPath("../include"),
.headerSearchPath("../../googletest/include"),
],
linkerSettings: [
.linkedLibrary("swiftCore", .when(platforms: [.windows])), // for swift_addNewDSOImage
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have to add this otherwise I get linker errors about "missing symbol swift_addNewDSOImage" -- @compnerd any chance you know why this is an issue on Windows (and WebAssembly) in particular?

Copy link
Member

Choose a reason for hiding this comment

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

It is because you are using swiftc as the linker driver. This will add in the swift registrar (swiftrt.obj or swiftrt.o) which requires that you overlink against swiftCore. I think that we want to build sqlite statically and not as a DLL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it. I think in the longer term we probably should change the build system to use swift as the linker driver only if there's any Swift sources built in the target, otherwise clang/clang++?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would make sense. We could also use -nostdlib as a flag for the driver to exclude the registrar and the runtime.

@jakepetroules
Copy link
Contributor Author

@swift-ci please test

@jakepetroules
Copy link
Contributor Author

@swift-ci please smoke test

Package.swift Outdated Show resolved Hide resolved
@jakepetroules
Copy link
Contributor Author

@swift-ci please smoke test

@jakepetroules
Copy link
Contributor Author

@swift-ci please test

@owenv
Copy link
Contributor

owenv commented Aug 7, 2024

The current round of CI failures should resolve once the new repo is added to update-checkout and related configs

jakepetroules added a commit to swiftlang/swift that referenced this pull request Aug 7, 2024
This is for swiftlang/swift-llbuild#930, which adds swift-toolchain-sqlite as a new dependency to SwiftPM to better support Windows and other platforms.
@jakepetroules
Copy link
Contributor Author

The current round of CI failures should resolve once the new repo is added to update-checkout and related configs

Opened swiftlang/swift#75743 for that

Copy link
Contributor

@owenv owenv left a comment

Choose a reason for hiding this comment

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

LGTM once CI is configured to check everything out

jakepetroules added a commit to swiftlang/swift that referenced this pull request Aug 7, 2024
This is for swiftlang/swift-llbuild#930, which adds swift-toolchain-sqlite as a new dependency to SwiftPM to better support Windows and other platforms.
This adds a dependency on the new swift-toolchain-sqlite package, which allows llbuild to build using `swift build` on Windows (including tests) without any additional flags or preinstalled dependencies.

rdar://133338086
jakepetroules added a commit to swiftlang/swift that referenced this pull request Aug 7, 2024
…5743)

This is for swiftlang/swift-llbuild#930, which adds swift-toolchain-sqlite as a new dependency to SwiftPM to better support Windows and other platforms.
@jakepetroules
Copy link
Contributor Author

@swift-ci please smoke test

@jakepetroules jakepetroules enabled auto-merge (rebase) August 8, 2024 00:34
@jakepetroules
Copy link
Contributor Author

@swift-ci please test Windows

1 similar comment
@jakepetroules
Copy link
Contributor Author

@swift-ci please test Windows

@jakepetroules jakepetroules requested review from dmbryson and removed request for dmbryson August 8, 2024 05:12
@jakepetroules jakepetroules enabled auto-merge (rebase) August 8, 2024 05:12
@jakepetroules jakepetroules merged commit d0e5ca7 into main Aug 8, 2024
3 checks passed
@jakepetroules jakepetroules deleted the windows branch August 8, 2024 05:44
@@ -233,6 +242,16 @@ let package = Package(
cxxLanguageStandard: .cxx14
)

if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] == nil {
Copy link

Choose a reason for hiding this comment

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

not sure if this is gated by a tools version thats too old, but I think you should be able to use Context.environment instead of pulling in Foundation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know that existed! I guess it's been there since Swift 5.6. But we should probably update this systematically across all apple/swiftlang repos, it appears in a lot of places.

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.

None yet

5 participants