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

deprecate LinuxMain.swift #3053

Merged
merged 2 commits into from
Nov 19, 2020
Merged

deprecate LinuxMain.swift #3053

merged 2 commits into from
Nov 19, 2020

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Nov 14, 2020 •
edited
Loading

motivation: Swift 5.1 introduced the --enable-test-discovery flag which instructs SwiftPM to discover tests instead of relying on a test manifest file named LinuxMain.swift. This functionality has been pretty well adopted and we are making it the default

changes:

  • deprecate --enable-test-discovery flag
  • deprecate --generate-linuxmain flag
  • automatically detect tests on non-darwin platforms
  • support "XCTMain.swift", "LinuxMain.swift" as escape hatch when test discovery is not appropriate. These files take priority.
  • package init no longer generates LinuxMain.swift
  • add and adjust tests

TODO

  • Add tests, functionality is not well covered in unit test. I manually tested though on Linux and works nicely.
  • Confirm behavior in marked with 👀
    - [ ] Make formal proposal before merging

@tomerd tomerd marked this pull request as draft November 14, 2020 18:31
@tomerd
Copy link
Contributor Author

tomerd commented Nov 14, 2020

background: https://forums.swift.org/t/pitch-enable-test-discovery-by-default/36619/20

@tomerd
Copy link
Contributor Author

tomerd commented Nov 14, 2020

cc @0xTim @compnerd

@tomerd
Copy link
Contributor Author

tomerd commented Nov 14, 2020 •
edited
Loading

@abertelrud @neonichu looking for feedback on approach. note the following TODOs listed ^^

tomerd
tomerd commented Nov 14, 2020
View reviewed changes
Sources/Build/BuildPlan.swift Outdated Show resolved Hide resolved
tomerd
tomerd commented Nov 14, 2020
View reviewed changes
Sources/Build/BuildPlan.swift Outdated Show resolved Hide resolved
MaxDesiatov
MaxDesiatov approved these changes Nov 14, 2020
View reviewed changes
Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Great stuff, overall LGTM. 👍

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Nov 14, 2020 •
edited
Loading

On a somewhat related note, I really wish SwiftPM truly supported the newly-introduced @main attribute. Then it could be used in test targets to configure XCTMain with test observers that output parseable test summaries, and whatever other customizations people might need. But this is obviously a completely separate thing from this PR, I only wish someone would consider it for implementation.

@0xTim
Copy link
Member

0xTim commented Nov 15, 2020

Haha I'd just started doing this on Friday! I'll take a look at the PR

0xTim
0xTim approved these changes Nov 15, 2020
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

LGTM!

@0xTim
Copy link
Member

0xTim commented Nov 15, 2020

@tomerd one thing that's just popped into my head - currently if you run swift build on a package that has a test target but no LinuxMain.swift you get a build error as you need to pass the --enable-test-discovery flag. This is a known bug that I'm assuming this fixes but I can't see an explicit test to ensure building a package works, so might be worth adding

@neonichu
Copy link
Contributor

LGTM, but as mentioned inline, we have to be careful about changing the testability behavior. SwiftPM itself started relying on it, so other packages might have, too.

@tomerd tomerd self-assigned this Nov 16, 2020
@tomerd tomerd force-pushed the feature/bye-bye-linux-main branch from a1c8cea to c16c62c Compare November 17, 2020 22:08
@tomerd tomerd marked this pull request as ready for review November 18, 2020 05:10
@tomerd tomerd force-pushed the feature/bye-bye-linux-main branch 2 times, most recently from bd30771 to 1127a39 Compare November 18, 2020 05:27
@tomerd
Copy link
Contributor Author

tomerd commented Nov 18, 2020

@swift-ci please smoke test

@tomerd tomerd force-pushed the feature/bye-bye-linux-main branch from 1127a39 to cb8cc2a Compare November 18, 2020 08:24
@tomerd
Copy link
Contributor Author

tomerd commented Nov 18, 2020

@swift-ci please smoke test

@neonichu
Copy link
Contributor

Linux error looks related:

01:23:49 error: signalled(4): /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-main/build/buildbot_incremental/swiftpm-linux-x86_64/x86_64-unknown-linux-gnu/release/swift-tools-support-corePackageTests.xctest --dump-tests-json output:
01:23:49 Fatal error: Use swift test --enable-test-discovery to run tests: file swift_tools_support_corePackageTests/LinuxMain.swift, line 1

@tomerd
Copy link
Contributor Author

tomerd commented Nov 18, 2020

@swift-ci please smoke test

@neonichu
Copy link
Contributor

Oh, there's an explicit fatalError in https://github.com/apple/swift-tools-support-core/blob/main/Tests/LinuxMain.swift

@tomerd
Copy link
Contributor Author

tomerd commented Nov 18, 2020

Oh, there's an explicit fatalError in https://github.com/apple/swift-tools-support-core/blob/main/Tests/LinuxMain.swift

yes, but I think we should have better backwards compatibility story for such use cases...added one

@tomerd
deprecate LinuxMain.swift
b748446 
motivation: Swift 5.1 introduced the --enable-test-discovery flag which instructs SwiftPM to discover tests instead of relying on a test manifest file named LinuxMain.swift. This functionality has been pretty well adopted and we are making it the default

changes:
* deprecate --enable-test-discovery flag
* deprecate --generate-linuxmain flag
* automatically detect tests on non-darwin platforms
* support "XCTMain.swift", "LinuxMain.swift" as escape hatch when test discovery is not appropriate. This files take priority.
* `package init` no longet generate LinuxMain.swift
* add and adjust tests

rdar://problem/64197288
rdar://problem/59534310
rdar://problem/62895076
@tomerd tomerd force-pushed the feature/bye-bye-linux-main branch from 0b3e891 to 5716ee4 Compare November 18, 2020 20:37
@tomerd
Copy link
Contributor Author

tomerd commented Nov 18, 2020

@swift-ci please smoke test

@tomerd tomerd force-pushed the feature/bye-bye-linux-main branch from 5716ee4 to 156d877 Compare November 18, 2020 20:39
@tomerd
Copy link
Contributor Author

tomerd commented Nov 18, 2020

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Nov 18, 2020

@abertelrud @neonichu this is ready for final review and merging

@neonichu
Copy link
Contributor

LGTM, thanks 👍

abertelrud
abertelrud approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

Looks good to me as well. Thanks!

@tomerd tomerd merged commit 7139eae into swiftlang:main Nov 19, 2020
@kateinoigakukun kateinoigakukun mentioned this pull request Jan 1, 2021
Closed
federicobucchi pushed a commit to federicobucchi/swift-package-manager that referenced this pull request Jan 6, 2021
@tomerd @federicobucchi
deprecate LinuxMain.swift (swiftlang#3053)
60189ec 
motivation: Swift 5.1 introduced the --enable-test-discovery flag which instructs SwiftPM to discover tests instead of relying on a test manifest file named LinuxMain.swift. This functionality has been pretty well adopted and we are making it the default

changes:
* deprecate --enable-test-discovery flag
* deprecate --generate-linuxmain flag
* automatically detect tests on non-darwin platforms
* support "XCTMain.swift", "LinuxMain.swift" as escape hatch when test discovery is not appropriate. This files take priority.
* `package init` no longet generate LinuxMain.swift
* add and adjust tests

rdar://problem/64197288
rdar://problem/59534310
rdar://problem/62895076
@tomerd tomerd mentioned this pull request Jan 20, 2021
Merged
@owenv owenv mentioned this pull request Mar 8, 2021
Merged
This was referenced Apr 26, 2021
Closed
Open
@nityanandaz nityanandaz mentioned this pull request Dec 19, 2021
Closed
@Bouke Bouke mentioned this pull request Dec 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abertelrud abertelrud abertelrud approved these changes

@0xTim 0xTim 0xTim approved these changes

@neonichu neonichu neonichu approved these changes

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

@aciidgh aciidgh Awaiting requested review from aciidgh

@friedbunny friedbunny Awaiting requested review from friedbunny

@compnerd compnerd Awaiting requested review from compnerd

Assignees

@tomerd tomerd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomerd @MaxDesiatov @0xTim @neonichu @abertelrud