Run CI on experimental *.md doc files

[joschmitt]

The CI doctests would not run on the examples included in any *.md file inside experimental as discovered in https://oscar-system.slack.com/archives/C5B2H11BQ/p1726048480750199 .
This tries to fix this by linking the experimental documentation into docs/src as part of the CI runners. Currently, the tests are expected to fail because of missing DocTestSetup = Oscar.doctestsetup() lines in some of the previously uncovered files. If this works and people are happy with my changes, I will fix those doctests.

[benlorenz]

Could we move the link_experimental_docs function to src/utils/docs.jl and call it from doctestsetup instead, something like this:

function doctestsetup()
  link_experimental_docs(Oscar)
  return :(using Oscar; Oscar.AbstractAlgebra.set_current_module(@__MODULE__))
end

This would simplify running this locally and from OscarDevTools.

[thofma]

I am puzzled about the missing setup. Shouldn't DocMeta.setdocmeta!(Oscar, :DocTestSetup, Oscar.doctestsetup(); recursive = true) take care of that?

Edit: It probably only takes care of the doctests attached to functions, but not to the doctests in the *md files?

[joschmitt]

I am puzzled about the missing setup. Shouldn't DocMeta.setdocmeta!(Oscar, :DocTestSetup, Oscar.doctestsetup(); recursive = true) take of that?

Yeah, I always wondered why we have to put this in every file.

[benlorenz]

setdocmeta! only applies to docstrings attached to functions and not to the markdown files. But I don't really know why it was designed like that.
There is an open issue for a better solution for the markdown files: JuliaDocs/Documenter.jl#2512

[joschmitt]

Could we move the link_experimental_docs function to src/utils/docs.jl and call it from doctestsetup instead, something like this:

function doctestsetup()
  link_experimental_docs(Oscar)
  return :(using Oscar; Oscar.AbstractAlgebra.set_current_module(@__MODULE__))
end

This would simplify running this locally and from OscarDevTools.

I tried this now. If I put the CI runners back to the original state, they don't test the *.md files if link_experimental_docs is only done with DocMeta.setdocmeta!(Oscar, :DocTestSetup, Oscar.doctestsetup(); recursive = true). The links are created, but I suppose they are created too late.

[benlorenz]

Could we move the link_experimental_docs function to src/utils/docs.jl and call it from doctestsetup instead, something like this:

function doctestsetup()
  link_experimental_docs(Oscar)
  return :(using Oscar; Oscar.AbstractAlgebra.set_current_module(@__MODULE__))
end

This would simplify running this locally and from OscarDevTools.

I tried this now. If I put the CI runners back to the original state, they don't test the *.md files if link_experimental_docs is only done with DocMeta.setdocmeta!(Oscar, :DocTestSetup, Oscar.doctestsetup(); recursive = true). The links are created, but I suppose they are created too late.

Did you maybe put the call inside the quoted expression? Otherwise this doesn't seem to make sense to me, but I will give it a try myself.

[joschmitt]

Could we move the link_experimental_docs function to src/utils/docs.jl and call it from doctestsetup instead, something like this:

function doctestsetup()
  link_experimental_docs(Oscar)
  return :(using Oscar; Oscar.AbstractAlgebra.set_current_module(@__MODULE__))
end

This would simplify running this locally and from OscarDevTools.

I tried this now. If I put the CI runners back to the original state, they don't test the *.md files if link_experimental_docs is only done with DocMeta.setdocmeta!(Oscar, :DocTestSetup, Oscar.doctestsetup(); recursive = true). The links are created, but I suppose they are created too late.

Did you maybe put the call inside the quoted expression? Otherwise this doesn't seem to make sense to me, but I will give it a try myself.

I pushed now what I have.

[benlorenz]

Thanks, I understand it now: Many markdown files have DocTestSetup = Oscar.doctestsetup() and the linking step will first clean up all links and recreate them so this might cause issues / race-conditions (In general it seems reasonable to clean up old / bad links first).

The issue with having extra code in the yml files is that it would need more extra treatment for Oscar to get this to run in the downstream tests in OscarDevTools.

[joschmitt]

I try to make the "cleaning up" step of link_experimental_docs a bit less aggressive.

[benlorenz]

We could also add a flag to make sure the linking is only done once.

[codecov]

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.65%. Comparing base (dbc2a03) to head (c2c2681).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/utils/docs.jl	90.90%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4083   +/-   ##
=======================================
  Coverage   84.65%   84.65%           
=======================================
  Files         614      614           
  Lines       83332    83365   +33     
=======================================
+ Hits        70543    70575   +32     
- Misses      12789    12790    +1     

Files with missing lines	Coverage Δ
src/utils/docs.jl	18.86% <90.90%> (+17.69%)	⬆️

... and 5 files with indirect coverage changes

[joschmitt]

Somehow nothing seems to work. I just pushed something to see the CI run. Please ignore it.

[joschmitt]

So the reason why the first attempt worked, but nothing else is this magical line in docs/make_work.jl:

# We monkey-patch Base.walkdir to use true as default value for follow_symlinks
# (normally false is the default), in order to "trick" the Documenter code into
# following those symlinks.
# See also:
# https://github.com/JuliaDocs/Documenter.jl/pull/552
# https://github.com/JuliaLang/julia/blob/master/doc/make.jl#L19
Base.walkdir(str::String) = Base.walkdir(str; follow_symlinks=true)

That's why it works if one includes docs/make_work.jl... 🤦‍♂️

[joschmitt]

Note that moving this to docs/documenter_helpers.jl should be fine because that file is included in docs/make_work.jl.

[joschmitt]

We could also add a flag to make sure the linking is only done once.

I think we introduced the deleting of all symlinks in #2215 because I started to switch branches and then some links weren't there anymore. With a global flag we would basically do the linking once per OSCAR session; I'm not sure that is stable in interactive use with Revise etc.?

For now, I edited link_experimental_docs to only delete links which shouldn't be there (and make sure that existing links are correct).

[joschmitt]

I have a version now which does not need to change the CI configuration. Could you have a look again @benlorenz?

[benlorenz]

Thanks for working on this! I think the test-code is good now.

Due to the walkdir issue this unfortunately doesn't work directly in OscarDevTools but should be simple enough to add by conditionally including documenter_helpers.jl (which I did not consider until now since it was only cosmetic).

[fingolfin]

Hmm, we only need this to make Documenter deal with symlinks to directories, right ? I think symlinks to files work, don't they?

That would suggest an alternative solution: recursively "closing" directories in the source by creating equally named dirs in the destination (if they are not there already) and then adding symlinks for all the files (and deleting stale symlinks). Quite a bit more work (though not really hard).

The main concern I have with the above is that Oscar.build_doc loads this code and then Base.walkdir is "tainted" in the whole session.

Of course this is existing code, so none of what I say here is relevant for merging this PR!

Anyway, I'll try and see if perhaps Documenter.jl is willing to just use follow_symlinks=true in their code, see JuliaDocs/Documenter.jl#2573

[fingolfin]

To be clear this is not meant to block this PR