-
Notifications
You must be signed in to change notification settings - Fork 631
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
initial adoption of DocC based documentaiton #2235
Conversation
motivation: use DocC for docs change: * add DocC catalog to NIO target, as the "landing page" * remove jazzy doc generation script * add Package.swift with tools-version 5.6 so that DocC can be used, and add a dependency on the DocC plugin * udpate soundness script
@@ -0,0 +1,183 @@ | |||
# ``NIO`` <!-- SwiftNIO --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated: should be doc be updated to cover async/await support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea,probably follow on PR or commits? this is all taken from the current readme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably follow on PR or commits?
yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Generally looks good, some notes in the diff.
* adjust waning-as-error and enable-test-discovery to the different permutations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, thanks!
Flaky test: |
@swift-nio-bot test this please |
@@ -1,4 +1,4 @@ | |||
// swift-tools-version:5.4 | |||
// swift-tools-version:5.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strangely, this change now requires that --enable-test-discovery
be passed when compiling the tests on linux/Android, or it errors like this with 5.6, 5.7, and trunk:
swift-nio/Tests/LinuxMain.swift:146:1: error: expressions are not allowed at the top level
(LinuxMainRunnerImpl() as LinuxMainRunner).run()
Anyone know what's going on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a bug in SwiftPM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I'll file a bug for SPM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lukasa Now that NIO supports 5.4 as a minimum can LinuxMain.swift not just be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly no. NIO tests with --warnings-as-errors
, and we have number of tests for deprecated code. The test discovery stubs in SwiftPM generate warnings when those tests are present up until 5.6, which is why our CI continues to use LinuxMain.swift
for 5.4 and 5.5. But we are at least finally on a path to dropping it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be fixed in 5.7 and main. but perhaps not available in the nightly toolchain yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fixed with the latest Aug. 9 trunk snapshot build for linux, but I'm using a June trunk snapshot because of another breaking regression for me. As you noted in that issue, should only be a problem with 5.6 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea will be hard to justify pulling into 5.6 given workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which workaround? Adding @main is a non-starter if someone is testing 5.3 and earlier, when the attribute wasn't there, swiftlang/swift@df99de8. As for overriding it with --enable-test-discovery
, adding that only for 5.6.3 for all future CI testing is going to be annoying and has other consequences.
motivation: use DocC for docs
change:
_