Skip to content

Comment out @testable and @_spi #394

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
Merged
merged 5 commits into from
Dec 11, 2020
Merged

Comment out @testable and @_spi #394

merged 5 commits into from
Dec 11, 2020

Conversation

davidungar
Copy link
Contributor

For now, comment out testable and _spi, which makes it easier to try the driver out with various versions of the compiler.
Someday, we can either uncomment the @SPI's, or we can move components into sub-modules. What do you think?

@davidungar
Copy link
Contributor Author

@swift-ci please test

@artemcm
Copy link
Contributor

artemcm commented Dec 7, 2020

This is much less than I thought.
We should make sure we test this against ToT of swift following these steps:
https://github.com/artemcm/swift-driver/#testing-against-swift-compiler-trunk

@davidungar davidungar mentioned this pull request Dec 8, 2020
Closed
@davidungar
Copy link
Contributor Author

This is much less than I thought.
We should make sure we test this against ToT of swift following these steps:
https://github.com/artemcm/swift-driver/#testing-against-swift-compiler-trunk

Great thought! Trying this at swiftlang/swift#35000

@davidungar
Copy link
Contributor Author

@swift-ci please test

@davidungar
Copy link
Contributor Author

@artemcm Looks like swiftlang/swift#35000 passed. Does this PR merit approval?

@davidungar
Copy link
Contributor Author

@swift-ci please test

owenv
owenv approved these changes Dec 11, 2020
View reviewed changes
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.

In hindsight, I probably shouldn't have started introducing @_spi into the codebase so early. This LGTM, and it should be easy to improve on once the feature is more stable

Tests/SwiftDriverTests/IncrementalCompilationTests.swift Outdated
@@ -12,7 +12,7 @@
import XCTest
import TSCBasic

@testable import SwiftDriver
/*@testable*/ import SwiftDriver
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can probably drop the @testable comments, I think we're better off never bringing them back assuming we can eventually start using @_spi again.

@davidungar
Copy link
Contributor Author

Thanks, @owenv !

@davidungar
Copy link
Contributor Author

@swift-ci please test

@davidungar davidungar merged commit 76b74cc into swiftlang:main Dec 11, 2020
@davidungar davidungar deleted the no-testables branch January 29, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owenv owenv owenv approved these changes

@artemcm artemcm Awaiting requested review from artemcm

@beccadax beccadax Awaiting requested review from beccadax

@DougGregor DougGregor Awaiting requested review from DougGregor

@CodaFi CodaFi Awaiting requested review from CodaFi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidungar @artemcm @owenv