Skip to content

Adopt swift-tools-protocols #2324

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Adopt swift-tools-protocols #2324

wants to merge 1 commit into from
+784 −22,198

Conversation

@owenv
Copy link
Contributor

@owenv owenv commented Oct 15, 2025

Adopt the new package and remove duplicated code

@owenv
Copy link
Contributor Author

owenv commented Oct 15, 2025

I expect I'll need to iterate on this a bit. swiftlang/swift#84882 needs to land before it's testable in CI

@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from cecc4f7 to c77d687 Compare October 21, 2025 23:20
@owenv
Copy link
Contributor Author

owenv commented Oct 21, 2025

@swift-ci test

@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from c77d687 to 6de2320 Compare October 22, 2025 16:59
@owenv
Copy link
Contributor Author

owenv commented Oct 22, 2025

@swift-ci test

@owenv
Copy link
Contributor Author

owenv commented Oct 22, 2025

@swift-ci test Windows

@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from 6de2320 to 31793f4 Compare October 22, 2025 19:40
@owenv
Copy link
Contributor Author

owenv commented Oct 22, 2025

@swift-ci test

@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from 31793f4 to 84b1416 Compare October 22, 2025 22:54
@owenv
Copy link
Contributor Author

owenv commented Oct 22, 2025

@swift-ci test

@owenv
Copy link
Contributor Author

owenv commented Oct 22, 2025

@swift-ci test Windows

@owenv owenv mentioned this pull request Oct 23, 2025
Open
@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch 10 times, most recently from 40a68e0 to 8f0f529 Compare October 28, 2025 20:01
@owenv owenv mentioned this pull request Oct 29, 2025
Open
@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from 8f0f529 to 83337ec Compare October 29, 2025 16:32
ahoppen
ahoppen reviewed Oct 30, 2025
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

High-level comment: I’d like to investigate if we can reduce the dependency on ToolsProtocolsSwiftExtensions further but that can be a follow-up investigation.

Sources/BuildServerIntegration/BuildTargetIdentifierExtensions.swift
}

extension BuildTargetIdentifier: CustomLogStringConvertible {
@_spi(SourceKitLSP) extension BuildTargetIdentifier: @retroactive CustomLogStringConvertible {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, should this conformance be in swift-tools-protocols? Seems like most people would need to add it. Though I’m not quite sure which module that conformance would live in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could put it in LanguageServerProtocolTransport although it's a bit of a stretch

Copy link
Member

Choose a reason for hiding this comment

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

In SourceKit-LSP, I’d put this in SKUtilities but I don’t think it’s worth adding a module in swift-tools-protocols just for this conformance. Let’s leave it for now and try to remember that if we should introduce such a module in swift-tools-protocols, we could move this conformance.

ahoppen
ahoppen reviewed Oct 30, 2025
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Sorry, hit submit to early

Sources/sourcekit-lsp/SourceKitLSP.swift
@@ -234,18 +234,20 @@ struct SourceKitLSP: AsyncParsableCommand {
fatalError("failed to redirect stdout -> stderr: \(strerror(errno)!)")
}

LoggingScope.configureDefaultLoggingSubsystem("org.swift.sourcekit-lsp")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is a little tricky. We’d need to do the same in at least one other places (part from test execution), namely when launching a SourceKitLSP server in-processes using InProcessClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the missing configure call to InProcessClient

Tests/BuildServerIntegrationTests/BuildServerManagerTests.swift Outdated
@@ -82,6 +83,10 @@ private func createBuildServerManager(
}

final class BuildServerManagerTests: XCTestCase {
override func setUp() async throws {
Copy link
Member

Choose a reason for hiding this comment

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

I really dislike having this setUp function in every single test case. Should we instead have a custom class that inherits from XCTestCase and that all our test cases inherit from?

Also, could we make LoggingScope.subsystem not fatalError if no default logging subsystem was set? Instead, maybe just use org.swift or default and log a fault that no logging subsystem has been set? There’s no need to crash the process just because no logging subsystem has been configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can get behind not crashing, swiftlang/swift-tools-protocols#16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also introduced an XCTestCase subclass for XCTests, and a suite trait for Swift Testing Tests

@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from 83337ec to 7c4b22b Compare October 30, 2025 21:36
@owenv
Copy link
Contributor Author

owenv commented Oct 30, 2025

swiftlang/swift-installer-scripts#474
swiftlang/swift#85078
@swift-ci test

@owenv
Copy link
Contributor Author

owenv commented Oct 30, 2025

swiftlang/swift-installer-scripts#474
swiftlang/swift#85078
@swift-ci test windows

@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from 7c4b22b to 9490f8a Compare October 31, 2025 03:04
@owenv
Copy link
Contributor Author

owenv commented Oct 31, 2025

swiftlang/swift-installer-scripts#474
swiftlang/swift#85078
@swift-ci test

@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from 9490f8a to d2e4b06 Compare October 31, 2025 03:24
@owenv
Copy link
Contributor Author

owenv commented Oct 31, 2025

swiftlang/swift-installer-scripts#474
swiftlang/swift#85078
@swift-ci test

ahoppen
ahoppen reviewed Oct 31, 2025
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

A couple small comments, otherwise LGTM.

Tests/SourceKitDTests/SourceKitDRegistryTests.swift Outdated
import SKTestSupport
import SourceKitD
import SwiftExtensions
import TSCBasic
@_spi(SourceKitLSP) import ToolsProtocolsSwiftExtensions
import XCTest

final class SourceKitDRegistryTests: XCTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t this also be SourceKitLSPTestCase? Are there any other lingering XCTestCase subclasses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this instance. There are a few unit tests of data structures and things which I left subclasses of SKTestCase to avoid adding an SKTestSupport dependency where it wasn't needed

@owenv owenv force-pushed the owenv/adopt-swift-tools-protocols branch from d2e4b06 to a7977d6 Compare October 31, 2025 17:58
@owenv owenv requested a review from ahoppen October 31, 2025 18:00
@owenv
Copy link
Contributor Author

owenv commented Oct 31, 2025

swiftlang/swift-installer-scripts#474
swiftlang/swift#85078
@swift-ci test

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

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

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

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@owenv @ahoppen