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.

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

[DRAFT] Partial docfx fix - Missing some plugins #958

Closed
wants to merge 1 commit into from

Conversation

nikcio
Copy link
Contributor

@nikcio nikcio commented Sep 13, 2024

This is a leg more on the fix @paulirwin have been trying out for solving #911

This is based on the commits @paulirwin made here: https://github.com/paulirwin/lucene.net/tree/issue/911

Use this to finish off the what is left to have docfx fully working.

What did I add?

  1. Replaced toc.yml
    As mentioned here there was a problem with the toc.yml being locked this was fixed by changing the path to "./toc.yml" instead of toc.yml".

luc-1

  1. Added small delay when building site output.

While the first fix solved the problem I noticed in one case when running locally that the file got locked anyway therefore I added a 1 second delay between starting new site out builds and haven't seen the error since.

  1. Fixed the links to the assets (js and css)

When starting the docs site locally the pages weren't showing the CSS or using the js files because the route was wrong due to the missing _rel variable, therefore, I've added a slash / before all uses of these files so it works locally and removed the ending slash from the docfx.global.subsite.json.

docfx.vendor.css was also changed to docfx.vendor.min.css. docfx.vendor.jswas also changed todocfx.vendor.min.js`

  1. Added an environment variable replacement postprocessor

I'm not sure if it works in all cases that it should but I've created a EnvironmentVariableProcessor that makes sure to replace EnvVar:ENVIRONMENT_VARIABLE to the actual value of the environment variable. As far as I could tell this is only used on the index page but I have no idea if this is correct. I haven't replaced the other plugin modules in these changes so that is still missing.

image

Next step

@paulirwin or @Shazwazza feel free to take these changes and finish them off 😄

I think the only thing missing is the remaining commented out plugin files otherwise it looks to be working.

How I've been testing

I've used the following command to test it.

./websites/apidocs/docs.ps1 -ServeDocs -LuceneNetVersion 4.8.0-beta00016 -BaseUrl http://localhost:8080

@paulirwin
Copy link
Contributor

Thanks for this! I'm going to merge this into an issue branch in this repo for collaboration. I was seemingly able to fix the toc locking by setting maxParallelism to 1. Obviously not ideal for performance, but reliability > performance right now, and it's still not too slow.

@paulirwin paulirwin self-assigned this Sep 15, 2024
@paulirwin
Copy link
Contributor

I have merged this with my work into this issue branch: https://github.com/apache/lucenenet/tree/issue/911

We can now use this branch to finish up collaboration on this issue. Note though that I am getting an error building your plugins changes, not sure why. However, it's great that running with -DisablePlugins now works for the libraries! This is a huge boost to progress, thank you. I'll be digging into this more soon.

@NightOwl888
Copy link
Contributor

This is great. That would explain why I saw it choke when the 3rd part JSON parser tried to read the ToC.

As for the version numbers, we ended up resorting to using Powershell to update them here:

# HACK: DocFx doesn't seem to work with fenced code blocks, so we update the lucene-cli index file manually.
# Note it works better this way anyway because we can store a real version number in the file in the repo.
(Get-Content -Path $CliIndexPath -Raw) -Replace '(?<=--version\s)\d+?\.\d+?\.\d+?(?:\.\d+?)?(?:-\w+)?', $LuceneNetVersion | Set-Content -Path $CliIndexPath
# Update our TOC to the latest LuceneNetVersion
(Get-Content -Path $TocPath1 -Raw) -Replace '(?<=lucenenet\.apache\.org\/docs\/)\d+?\.\d+?\.\d+?(?:\.\d+?)?(?:-\w+)?', $LuceneNetVersion | Set-Content -Path $TocPath1
(Get-Content -Path $TocPath2 -Raw) -Replace '(?<=lucenenet\.apache\.org\/docs\/)\d+?\.\d+?\.\d+?(?:\.\d+?)?(?:-\w+)?', $LuceneNetVersion | Set-Content -Path $TocPath2
# Update the API link to the latest LuceneNetVersion
# Note we don't update _rel because that is used for styles and js
(Get-Content -Path $BreadcrumbPath -Raw) -Replace '(?<="_api":\s*?"https?\:\/\/lucenenet\.apache\.org\/docs\/)\d+?\.\d+?\.\d+?(?:\.\d+?)?(?:-\w+)?', $LuceneNetVersion | Set-Content -Path $BreadcrumbPath
. We tried several times to get the plugins to do that for us, but they don't seem to work in any section of the page, only in certain sections (for example, we couldn't replace the version number inside of a code block using the plugin).

@paulirwin
Copy link
Contributor

I think we should be able to do that with plugins now as a post-processor! IIUC, anyways.

@paulirwin
Copy link
Contributor

I was able to fix the plugin build error by excluding some files that were using the old docfx SDK. I went ahead and excluded the rest of those files that were commented out, no reason in keeping them in the csproj. By the time we merge this we should remove those from source control (they will of course always be available in prior version history if needed).

I'm testing this now and will review the rest of the plugin code to see what all is needed to be implemented. This is very exciting to use the latest docfx!

@paulirwin
Copy link
Contributor

I added a post-processor for the @lucene "note" blocks, and exported a single aggregate post processor to not have to update all of the JSON files for each one if we add any more in the future. (I also was running into an error about not allowing multiple post-processor exports, but that might have been tired-eyed PEBKAC last night.)

CleanShot 2024-09-16 at 07 40 53@2x

https://github.com/apache/lucenenet/tree/issue/911

Next up, I'll look into seeing if the things we were doing in PowerShell can be done in a post-processor instead, since we have much more control over the output now.

@NightOwl888
Copy link
Contributor

Hmm...the whole thing is in Powershell, except for the DocFx bit :). I suggest you have a look at the Docs and Website Documentation and the build automation for the docs and the website. They are set up to build the docs and then submit a PR to the lucenenet-site repo (although, the trigger isn't working and it requires starting manually).

Also, if you checkout the tag for beta00016, you should be able to run the build prior to the DLL conflict to see it work. Ideally, the UX and procedure won't have to change for the upgrade.

@paulirwin
Copy link
Contributor

Sorry, I was meaning replacing the "HACKs" you were referring to here: #958 (comment)

We at least should be able to do that for the CLI index. I'll look into it.

@paulirwin
Copy link
Contributor

The CLI docs version replacement was an easy change, made that use EnvVar:LuceneNetVersion since that works in fenced code blocks now. Removed that from the ps1 script; the rest are TOC/breadcrumbs replacements that I think should stay for now.

Next up I'll be looking into why the header links aren't working, but once that is working we should be in good shape for the API docs site.

@paulirwin
Copy link
Contributor

@nikcio I have included your commit in #961 so I'll close this PR. Thanks!

@paulirwin paulirwin closed this Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants