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

Bugfixes to Documenter config, docs links #166

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Conversation

tsj5
Copy link
Collaborator

@tsj5 tsj5 commented Jun 8, 2022

This PR fixes two separate bugs which raise warnings when building documentation, as described in issue #167. Thanks to @charleskawczynski for helping me debug this issue!

  1. The first bug is that relative paths are not supported as targets of Documenter @ref links (Documenter issue 509 -- I think the link is generated correctly but doesn't pass the linkcheck stage, but don't quote me on that.)
    The fix implemented here is the same as what was used for the julialang docs: the link targets are explicitly marked with an @id tag, and the @ref tag is changed to refer to that @id tag, rather than the file's path.
    Ironcially, this could be better documented in the Documenter docs themselves...

  2. The second bug only takes place when building docs locally, and results in missing file warnings for files that exist. This is due to Literate.jl, which generates non-functional URLs (containing the string <unkown>) if the config variable repo_root_url is not set. Literate.jl's default configuration picks this up correctly when run under common CI services (eg. GH Actions), but not when built locally.
    The fix implemented here is to manually set this path in make.jl only when being run outside of CI (which we test for by testing the existence of a shell environment variable named CI).

With the two fixes above, documentation builds locally without raising any warnings, and manually checking confirms the links in question are generated correctly.

@tsj5 tsj5 changed the title Documentation bugfix [WIP] Documentation bugfix Jun 8, 2022
@tsj5 tsj5 self-assigned this Jun 8, 2022
@tsj5 tsj5 linked an issue Jun 9, 2022 that may be closed by this pull request
@odunbar
Copy link
Collaborator

odunbar commented Jun 13, 2022

┌ Warning: invalid local link: unresolved path in ensemble_kalman_inversion.md
│   link.text =
│    1-element Vector{Any}:
│     "Prior distributions"
│   link.url = "parameter_distributions.html"
└ @ Documenter.Writers.HTMLWriter ~/.julia/packages/Documenter/MLty7/src/Writers/HTMLWriter.jl:2081

In the latest GH run i still see this warning. (and the other invalid local links)

@tsj5
Copy link
Collaborator Author

tsj5 commented Jul 2, 2022

bors try

bors bot added a commit that referenced this pull request Jul 2, 2022
@bors
Copy link
Contributor

bors bot commented Jul 2, 2022

try

Build succeeded:

@tsj5
Copy link
Collaborator Author

tsj5 commented Jul 2, 2022

┌ Warning: invalid local link: unresolved path in ensemble_kalman_inversion.md
│   link.text =
│    1-element Vector{Any}:
│     "Prior distributions"
│   link.url = "parameter_distributions.html"
└ @ Documenter.Writers.HTMLWriter ~/.julia/packages/Documenter/MLty7/src/Writers/HTMLWriter.jl:2081

In the latest GH run i still see this warning. (and the other invalid local links)

I was misunderstanding the problem in the earlier version of this PR -- this should be completely fixed.

@tsj5 tsj5 requested a review from odunbar July 2, 2022 19:39
@odunbar
Copy link
Collaborator

odunbar commented Jul 7, 2022

LGTM! Just need to merge from main.

Use current EKP as module when building docs locally

Replace relative path @refs with explicit @id tags
@tsj5 tsj5 changed the title [WIP] Documentation bugfix Bugfixes to Documenter config, docs links Jul 20, 2022
@tsj5
Copy link
Collaborator Author

tsj5 commented Jul 20, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 20, 2022

Build succeeded:

@bors bors bot merged commit 5d28276 into CliMA:main Jul 20, 2022
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.

Fix warnings in building docs
2 participants