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.

Sign up for GitHub

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

Jump to bottom

Sever TSC/Driver dependencies if using SwiftBuild Framework #8442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Sever TSC/Driver dependencies if using SwiftBuild Framework #8442

wants to merge 1 commit into from

Conversation

bkhouri
Copy link
Contributor

@bkhouri bkhouri commented Apr 2, 2025 •
edited by pmattos
Loading

The SwiftBuild project has a dependency on Swift Driver, which has a dependency on Swift Tools Support Core (STSC). Swift Package Manager (SwiftPM) executable targets also have a dependency on STSC.

Sever SwiftPM package dependency on STSC if the
SWIFTPM_SWBUILD_FRAMEWORK environment variable is set so we can indirectly pull the STCS dependency on the dynamic library that will be pulled in via SwiftDriver.

Reverts #8437

MaxDesiatov
MaxDesiatov reviewed Apr 2, 2025
View reviewed changes
@bkhouri bkhouri marked this pull request as draft April 2, 2025 14:23
@bkhouri bkhouri force-pushed the revert-8437-revert-8434-t/main/sever_swift-support-core_dependency branch 2 times, most recently from f22279f to 37c4690 Compare April 2, 2025 14:31
@MaxDesiatov MaxDesiatov changed the title Revert "Revert "Sever the package dependency if using SwiftBuild Framework"" Sever TSC/Driver dependencies if using SwiftBuild Framework Apr 2, 2025
@MaxDesiatov
Copy link
Contributor

Updated PR title to be more readable, as the previous "the package dependency" qualifier did not make it clear at a glance which exact dependencies are gone.

@MaxDesiatov MaxDesiatov added the dependencies Changes to dependencies and relevant checks label Apr 2, 2025
bkhouri
bkhouri commented Apr 3, 2025
View reviewed changes
Package.swift Outdated
@@ -87,6 +87,26 @@ if ProcessInfo.processInfo.environment["SWIFTCI_INSTALL_RPATH_OS"] == "android"
*/
let autoProducts = [swiftPMProduct, swiftPMDataModelProduct]

let shouldUseSwiftBuildPackageDependency = (
Copy link
Contributor Author

@bkhouri bkhouri Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

chore (blocking): Need to ensure this does not produce #8437 (comment)

Building SwiftPM with swift build, using SWIFTPM_NO_SWBUILD_DEPENDENCY=1 and SWIFTCI_USE_LOCAL_DEPS=1

.../swiftpm/Sources/Basics/Archiver/TarArchiver.swift:15:15: error: no such module 'TSCBasic'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drodriguez : Could you please try this change to ensure it does not break your workflow?

Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the builds for people that chosen to use SWIFTPM_NO_SWBUILD_DEPENDENCY and avoid swift-build. After this change it seems that the swift-build dependency is required. Is that one of the intents of the change? The summary seems to talk about changing things when the swift-build framework is used, but nothing about requiring swift-build for every build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SwiftBuild is intended to be made available via the --build-system swiftbuild option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since #8373 was merged, SWIFTPM_NO_SWBUILD_DEPENDENCY is no longer required.

@bkhouri bkhouri force-pushed the revert-8437-revert-8434-t/main/sever_swift-support-core_dependency branch from 37c4690 to a610518 Compare April 4, 2025 18:10
@bkhouri
Copy link
Contributor Author

bkhouri commented Apr 4, 2025

@swift-ci test

@bkhouri bkhouri force-pushed the revert-8437-revert-8434-t/main/sever_swift-support-core_dependency branch from a610518 to 0a51cce Compare April 4, 2025 20:07
@bkhouri
Revert "Revert "Sever the package dependency if using SwiftBuild Fram…
44135cc 
…ework" (…"

This reverts commit e64232e, with
some changes
@bkhouri bkhouri force-pushed the revert-8437-revert-8434-t/main/sever_swift-support-core_dependency branch from 0a51cce to 44135cc Compare April 4, 2025 20:10
@bkhouri
Copy link
Contributor Author

bkhouri commented Apr 4, 2025

@swift-ci please test

@bkhouri
Copy link
Contributor Author

bkhouri commented Apr 4, 2025

@swift-ci please test self hosted windows

drodriguez
drodriguez reviewed Apr 5, 2025
View reviewed changes
Package.swift
@@ -1050,8 +1063,7 @@ if ProcessInfo.processInfo.environment["ENABLE_APPLE_PRODUCT_TYPES"] == "1" {
}
}

if ProcessInfo.processInfo.environment["SWIFTPM_SWBUILD_FRAMEWORK"] == nil &&
ProcessInfo.processInfo.environment["SWIFTPM_NO_SWBUILD_DEPENDENCY"] == nil {
if !shoudUseSwiftBuildFramework {
Copy link
Contributor

Choose a reason for hiding this comment

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

Recovering && ProcessInfo.processInfo.environment["SWIFTPM_NO_SWBUILD_DEPENDENCY"] == nil seems to allow the previous behaviour of disabling swift-build completely from SwiftPM.

Copy link
Contributor Author

@bkhouri bkhouri Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The SWIFTPM_NO_SWBUILD_DEPENDENCY was a temporary workaround until we were able to properly integrate SwiftBuild in the toolchain build. I don't believe it's required anymore since #8373 was merged

Copy link
Contributor

Choose a reason for hiding this comment

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

That's unfortunate. It was really helpful for workflows that did not want to use SwiftBuild just yet until all the dust settled. The SwiftPM code seems prepared to work without it, and as the small modification proves, it doesn't seem really necessary for SPM to work.

Copy link
Member

Choose a reason for hiding this comment

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

That code should be dormant until we make it the default build system and that will likely take quite a while yet.

@bkhouri
Copy link
Contributor Author

bkhouri commented Apr 7, 2025

@swift-ci test windows

@bkhouri
Copy link
Contributor Author

bkhouri commented Apr 7, 2025

@swift-ci test self hosted windows

@bkhouri bkhouri marked this pull request as ready for review April 7, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

@jakepetroules jakepetroules Awaiting requested review from jakepetroules jakepetroules is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

@dschaefer2 dschaefer2 Awaiting requested review from dschaefer2 dschaefer2 is a code owner

@shawnhyam shawnhyam Awaiting requested review from shawnhyam shawnhyam is a code owner

@bripeticca bripeticca Awaiting requested review from bripeticca bripeticca is a code owner

@plemarquand plemarquand Awaiting requested review from plemarquand plemarquand is a code owner

@owenv owenv Awaiting requested review from owenv owenv is a code owner

@cmcgee1024 cmcgee1024 Awaiting requested review from cmcgee1024 cmcgee1024 is a code owner

@daveyc123 daveyc123 Awaiting requested review from daveyc123 daveyc123 is a code owner

@drodriguez drodriguez Awaiting requested review from drodriguez

@MaxDesiatov MaxDesiatov Awaiting requested review from MaxDesiatov MaxDesiatov is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
dependencies Changes to dependencies and relevant checks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bkhouri @MaxDesiatov @drodriguez @dschaefer2