Merge from upstream

[hassila]

Handfull of bug fixes...

[franklinsch]

Hi @hassila, thanks so much for opening this! Have you performed testing for these changes, and if so, in what capacity? It would also be great to trigger cross-repo CI testing for Swift, Swift-DocC, and Swift-LMDB. There are instructions here: https://github.com/apple/swift/blob/main/docs/ContinuousIntegration.md#cross-repository-testing

[hassila]

Hi @franklinsch, we're just in the process of integrating LMDB support, so no extensive testing of the changes as of yet (just wanted to open up the discussion on keeping CLMDB up-to-date as it said on the tin, otherwise we'd consider wrapping it ourselves which seemed counterproductive).

I can see how I could possible get cross-repo CI testing if I open up a Swift-DocC PR to test against this one (although I wouldn't be able to trigger the build for it likely, but we'll see), but for "Swift" it's not obvious that there's any dependency as far as I could find? I must be missing something.

We'll test it internally with our own storage layer built on top although the unit test suite is fairly basic as of yet. I can also try running lbmd:s tests, but they are also fairly basic...

[franklinsch]

The test trigger would be in the Swift repo, with Swift-LMDB as a cross-repo reference. I can do that since I think you're right, you won't have the access to do so. Is this PR ready for testing?

[hassila]

Yes, it's as ready as I can make it - I've manually run the mdb test suite which look ok and also run the internal unit tests we have for our storage layer which also was ok. I did a fork/merge at https://github.com/hassila/swift-lmdb/tree/hassila-mdb-merge-patch as it made testing easier for me at least, I can open a new PR from there instead if you like?

[franklinsch]

Sounds good, thanks! I kicked off some builds here: swiftlang/swift#59324

[hassila]

Seems something broke, but must admit it's a bit unclear what the root cause was (I'm probably just missing how to navigate the CI results)

[franklinsch]

You can access the full log by clicking "View as plain text" (if the output is too large, you can curl it). It seems like this is the issue for the three failing pipelines:

Test Suite 'NavigatorIndexingTests' started at 2022-06-09 00:41:55.026
Test Case '-[SwiftDocCTests.NavigatorIndexingTests testNavigatorIndexAsReadOnlyFile]' started.

/Users/ec2-user/jenkins/workspace/swift-PR-toolchain-macos/branch-main/swift-docc/Sources/SwiftDocC/Indexing/Navigator/NavigatorIndex.swift:166: error: -[SwiftDocCTests.NavigatorIndexingTests testNavigatorIndexAsReadOnlyFile] : failed: caught error: "accessError"
Test Case '-[SwiftDocCTests.NavigatorIndexingTests testNavigatorIndexAsReadOnlyFile]' failed (1.298 seconds).

This seems related to the LMDB changes here. You should be able to reproduce the issue by cloning the swift-docc repo and updating the Package.swift file to use this LMDB branch instead, and running ./bin/test. Let me know if I can help with that!

[hassila]

Thanks @franklinsch, I tried on macOS with Swift 5.6.1 and passed main DocC test suite without any problems, then I pointed it to the updated LMDB branch and managed to reproduce one failure;

Test Case '-[SwiftDocCTests.NavigatorIndexingTests testNavigatorIndexAsReadOnlyFile]' started.
/Users/hassila/GitHub/swift-docc/Sources/SwiftDocC/Utility/LMDB/LMDB+Environment.swift:90: error: -[SwiftDocCTests.NavigatorIndexingTests testNavigatorIndexAsReadOnlyFile] : failed: caught error: "accessError"
Test Case '-[SwiftDocCTests.NavigatorIndexingTests testNavigatorIndexAsReadOnlyFile]' failed (0.706 seconds).

Basically LMDB fails to open the LMDB environment in readNavigatorIndex with an accessError when the file has been changed to read-only. I validated that the file was readable from the command line while breaking in the debugger and file permissions were appropriately changed - also commenting out the change of permissions of the file and the test passes fine, so there's some issue of opening an environment using readonly/nolocks. Will see if I can understand root cause.

[hassila]

Ok, the offending commit seems to be da0527a - specifically the removal of line 5616 if (!(flags & (MDB_RDONLY|MDB_WRITEMAP))) {. I will file an issue upstream.

[hassila]

Ok, I filed https://bugs.openldap.org/show_bug.cgi?id=9861 - I also tried a patch in https://github.com/hassila/swift-lmdb/tree/hassila-mdb-merge-patch - if I run the DocC test suite against that version it comes through clean FWIW.

Hopefully they will accept that fix and merge it upstream, we'll see - I'm not familiar enough with the code base to feel fully confident that the fix is exactly correct, so perhaps await feedback there first.

[hassila]

Ok, it was accepted and fixed upstream - can we kick off a new test run perhaps?

[franklinsch]

That's amazing, thanks so much for investigating, putting up a patch, and proposing it upstream! I triggered a new round of testing.

[hassila]

All passed :-)

[franklinsch]

Woo! 🎉 I also tested a local build and it's working great. Thanks so much @hassila!

[franklinsch]

Cross-repo tests passed here: swiftlang/swift#59324