Docfx Documentation Fixes, Upgrade docfx, Cross-platform support #961
Conversation
… prevent incorrect links to core, add slash after _rel for search worker
…xtractSearchIndex post-processor
|
|
…site build and upgrade to latest docfx
|
|
|
|
|
|
|
@NightOwl888 I converted your comment above into a checklist, and I'm checking off those issues as I go. Several of them have been resolved. Notably, I had to work around those homepage links going to test-framework by intentionally not including the test-framework xrefmap in the top-level API docs docfx.site.json, and linking to it manually instead. No amount of reordering the xrefmaps array seemed to make a difference. Unless we want to change those namespaces in TestFramework to not conflict with those in Core, I think we'll need to live with this workaround for now. |
I don't recall exactly why the extension methods were placed in the |
|
Yeah, it's more than just the RandomExtensions though. TestFramework has types in the Analysis, Codecs, and Search namespaces that conflict with the links on the homepage. Basically, a |
Namespaces are meant to be extended by other assemblies. But for some reason, the default setup of docfx doesn't understand that very well. They apparently made some changes to the way metadata is handled that are not compatible with our config that worked on the old version. But it was never great - in the old version links between subsites wouldn't resolve, so I had to hard code them. Now it seems that links are going to the wrong subsite so they have to be hard coded or painstakingly researched. As sad as it is, hard coding is a solution until we have time to get into the issues with why it fails to resolve many links most of the time. At least now it is spitting out warnings about all of the hard coded URLs so we can easily see what to fix later, so I guess that is an improvement. |
|
|
|
|
|
|
|
Alright all but issue 14 are resolved, not sure what's up with that. I resolved several xref map warnings/issues. There are a few xrefs that are not causing build warnings that aren't getting made into links for some reason, not sure why. An example are the ones on the OpenNLP overview page. But this is a great improvement so far. I think we can break out some of these issues separately. The primary issue (that docs cannot build) is resolved, and several doc issues have been fixed in this PR, so I think it's largely a net improvement. @NightOwl888 please review and let me know if this is good to merge, with breaking out the remaining issues. |
|
|
There was a problem hiding this comment.
It ran fine after fixing the malformed JSON in docfx.demo.json.
- There was no feedback that it was doing anything when I ran the docs.ps1 command - it simply froze for a long time. I was in the process of opening a new Powershell window to launch it anew when the first window finally came to life. A message there would be helpful, but it is not critical.
- The "Improve this doc" link is going to my fork of the repository instead of the apache repository. I don't know if that is a feature or a bug.
https://github.com/NightOwl888/lucenenet/blob/issue/911/src/Lucene.Net/overview.md/#L1 - In this block: http://localhost:8080/api/core/Lucene.Net.Analysis.html#attribute-and-attributesource, the table is missing the entire first column with the attributes and links. This seems like important info that is missing. It looks like several other tables are failing to resolve links, but this one is stripping them completely.
- I don't know if we can mark issue 8 closed per se. Although the first issue was fixed, the formulas are still screwy. But they always were.
- It is difficult to remember where you have been on the API docs if the link visited text is the same as the unvisited text, which was a bit annoying when trying to visit all of the links for a test.
I agree this is an improvement as there are many more links that now resolve that didn't previously than links that no longer resolve. All said, there is nothing critical I can see that is so urgent as to hold up the release.
If you wish to address any of the above issues, feel free, but this is fine to merge.
|
Hold up, it looks like lots of tests failed on Linux. I think it is a transient CI issue, though. I am going to try re-running them. |
|
The error is: It looks like .NET 5 requires OpenSSL 1.0 or 1.1, but some Linux distros only come with 3.1. https://discussions.unity.com/t/workaround-for-libssl-issue-on-ubuntu-22-04/879165/3 Apache has their own build agents, so it could be something INFRA could address or we could try to add it to the build. It is not likely related to this PR, though. This is likely something to do with the fact we target ubuntu-latest, although I cannot find any info on when they last updated to a newer version. Just that it is now 24.04. |
|
Here it is: actions/runner-images#10636. Looks like that is exactly what the issue is, as they are rolling that change out now. |
|
Want me to downgrade to 22.04 for now? |
|
I'm adding a message to communicate when it's restoring the docfx tool, which was the hang you saw. Regarding the "Improve this doc" link, that goes to my fork on my machine, so I think this might be okay when built against the main repo. I have added the remaining issues as #969 |
|
|
This will need to be propagated to all of our builds that are using .NET 5, both on GitHub Actions and Azure DevOps. I noticed .NET 5 failures earlier with ICU4N, but now that I am seeing them here it makes sense. Yes, I think the quickest solution would be to pin the OS version for the .NET 5 tests, but we should probably update both CI builds in a separate PR. |
|
Sounds good, I'll do that in a separate PR. |
This perspective made me very happy. |
This PR fixes several issues with the API docs and upgrades docfx.
Fixes #911
Thanks to @nikcio for the assistance.
Description