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

Remove LibGit2 dep #167

Closed
wants to merge 3 commits into from
Closed

Remove LibGit2 dep #167

wants to merge 3 commits into from

Conversation

nhz2
Copy link

@nhz2 nhz2 commented Aug 13, 2024

Alternative to #165

This only supports GitHub URLs right now. Ultimately it would be nice if this could somehow hook into https://documenter.juliadocs.org/stable/lib/remote-links/#remotes-for-files

Copy link

@ChrisRackauckas ChrisRackauckas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works too.

src/utilities.jl Outdated Show resolved Hide resolved
@mortenpi
Copy link
Member

Base.url can return file:// URLs something, which I don't think we want, and the old implementation just ignores those cases:

julia> Base.url(first(methods(Revise.retry)))
"file:///home/mortenpi/.julia/packages/Revise/uvGMC/src/packagedef.jl"

So my suggestion is that we keep doing that here. I took the liberty of pushing a change in 245a9cf to that effect.

@mortenpi
Copy link
Member

One question to anyone who knows the code loading better: is LibGit2 always loaded in a normal Julia session? Because it's no longer in the package environment, so I would expect this not to work:

$ julia --project -E'using DocStringExtensions; Base.url(first(methods(DocStringExtensions.template)))'
"https://github.com/JuliaDocs/DocStringExtensions.jl/tree/82e4fcdbad2d24aca5acbba7a1c7230567c7a0a7//src/templates.jl#L77"

But it clearly does.

The case I'm wondering if we should be making sure that LibGit2 gets loaded in Documenter, to guarantee that these links get correctly generated in the manual. Or it's fine, as long as you're building the docs with a normal Julia binary..?

Base.url: https://github.com/JuliaLang/julia/blob/96fd25a764c316e9e09dfd7de8e84d854a73b2cd/base/methodshow.jl#L374-L412

@KristofferC
Copy link
Member

KristofferC commented Aug 13, 2024

LibGit2 is in sysimage in 1.10. In 1.11 it doesn't work anymore (LibGit2 is not in sysimage).

@fredrikekre
Copy link
Member

Base.url can perhaps use CLI git and only try libgit2 otherwise. On CI git should always be configured correctly I think.

@mortenpi
Copy link
Member

Base.url can perhaps use CLI git and only try libgit2 otherwise.

It doesn't do that right now, and any fix upstream would also only be in 1.12+. So should we add that handling here? I.e. if Base.url doesn't return a proper URL, we'll try to see if git is available and is able to tell us something?

@MichaelHatherly
Copy link
Member

So should we add that handling here? I.e. if Base.url doesn't return a proper URL, we'll try to see if git is available and is able to tell us something?

That seems the most reasonable behaviour to me. Keeps the current behaviour in CI where we expect git to be available.

@nhz2 nhz2 changed the title Remove LibGit2 dep and fix test Remove LibGit2 dep Aug 16, 2024
@nhz2
Copy link
Author

nhz2 commented Sep 13, 2024

Since Base.url might break in 1.11, I'm closing this PR, since this seems to me like the wrong approach, at least for now. I also don't know enough about the command line git to implement the fallback. Feel free to reopen, or make a new PR.

@nhz2 nhz2 closed this Sep 13, 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.

6 participants