Remove linux-specific code and enable tests that pass on linux #6
Conversation
There was a problem hiding this comment.
Hi @buttaface! Thank you for opening this!
This looks generally ready-to-go. I just left a couple of comments inline for pieces I think we should consider splitting into separate PRs to reduce risk and get the majority of this merged as soon as possible.
And like with PR #5, we'll likely want to hold off on merging this until we initially land DocC in a trunk toolchain. But can definitely plan on can merging it immediately after.
| let results: [(reference: ResolvedTopicReference, content: RenderReferenceStore.TopicContent)] = references.concurrentPerform { reference, results in | ||
| results.append((reference, renderContentFor(reference))) | ||
| } | ||
| for result in results { | ||
| topics[result.reference] = result.content | ||
| } | ||
|
|
||
| #elseif os(Linux) |
There was a problem hiding this comment.
I just filed https://bugs.swift.org/browse/SR-15385 to track this and the other places in DocC where we've explicitly disabled parallelization on Linux (and now Android).
If you're okay with splitting out this work, I'd prefer to address this in another PR. We've seen unusually high memory usage when converting large documentation catalogs in parallel on Linux which is why this is disabled. Definitely would like to enable parallelizing documentation conversion on Linux but I think we have more investigation to do before then.
There was a problem hiding this comment.
Alright, reverted this.
| @@ -700,12 +700,6 @@ class ConvertServiceTests: XCTestCase { | |||
| } | |||
|
|
|||
| func testReturnsRenderReferenceStoreWhenRequestedForOnDiskBundleWithUncuratedArticles() throws { | |||
| #if os(Linux) | |||
There was a problem hiding this comment.
https://bugs.swift.org/browse/SR-15036 is still open so I think we should leave this disabled for now. I believe we were only seeing this issue on certain Linux distros like Ubuntu 18.04.
I'd be happy to file a bug blocked on SR-15036 to re-enable these once we resolve that issue.
There was a problem hiding this comment.
Sure, added these checks back.
There was a problem hiding this comment.
Thank you @buttaface! This looks great.
As with #5, I’ll plan on running CI once https://bugs.swift.org/browse/SR-15362 lands in a toolchain.
I’ll then merge this PR once CI has passed and we’ve merged the integration of DocC in the Toolchain with swiftlang/swift#39723.
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
Summary
These linuxisms were not needed on Fedora Core 33.
Dependencies
None
Testing
Steps:
./bin/teston linux with Swift 5.5 installed.Checklist
./bin/testscript and it succeededI noted that all these weren't needed on Android in #5, so I checked on Fedora Core 33 x86_64 and they weren't needed on linux either.
Note that I have not looked into why any of these conditional compilation checks for linux were originally added, I just know that they make no difference in getting the tests passing. If some of these should stay the same, just let me know.