Use GitHubActionsLogger when building docs

[ericphanson]

This lets us get inline annotations for doctest errors:

(well, that requires a small fix in Documenter and in GitHubActions.jl as well).

Tested here: https://github.com/ericphanson/VisualStringDistances.jl/runs/3450753006?check_suite_focus=true

Not sure if this should behind a setting (or opt-out), and if the use of the global env is the right way to go. (One way to opt-out though is to just set whatever logger you want yourself inside docs/make.jl -- so I think it kind of makes sense that the default should be a GitHubActionsLogger rather than a ConsoleLogger).

[ericphanson]

bump

[SaschaMann]

I don't know if using the global env is ideal either. Using the GHA logger seems like a sensible default to me, I'd say we can add an opt out if someone complains? :D

[ericphanson]

Yeah-- I think probably the global env is better than messing with the users local environment, though maybe we could do something like make a new environment in a tempdir, install the logger there, and then put that env in the JULIA_LOAD_PATH, and run the docs build with that? Then maybe even if the user is doing something with the global env elsewhere in the workflow we aren't messing with that either.

[SaschaMann]

Sounds good to me though I don't know pkg workflows like that well enough to understand if there are any potential side effects.

[SaschaMann]

Should we merge this as is or switch to the temporary env like you suggested? I'm happy with either

[ericphanson]

I was thinking about that too— I think merging is fairly safe in that it’s unlikely folks are using the global env in a way in other steps of their job that will clash with us installing the low-dependency GitHubActions.jl, and that the fancier solution (temp env pushed into the load path) has the risk of bugs since it is more complex and I understand it less.

However I’m pretty sure it’s the more “correct” approach in that it’s more isolated.

so… I’m in favor of just merging because I think it’ll be fine and we can address problems if they arise. But if other people would prefer the more isolated approach I can do that.

[DilumAluthge]

I don't feel particularly strongly, but I would lean towards "temp environment pushed to the load path".

[DilumAluthge]

Instead of pushing to the load path, you could just Pkg.activate the temp environment while you build the docs.

[ericphanson]

The most isolated thing would be to do what VSCode does and vendor the code of GitHubActions.jl (and it's dependency, JSON.jl) to avoid participating in the package environment altogether. But that seems a bit extreme...

[ericphanson]

Instead of pushing to the load path, you could just Pkg.activate the temp environment while you build the docs.

We need to activate the docs environment though

[DilumAluthge]

Instead of pushing to the load path, you could just Pkg.activate the temp environment while you build the docs.

We need to activate the docs environment though

Ah fair enough.

[DilumAluthge]

The most isolated thing would be to do what VSCode does and vendor the code of GitHubActions.jl (and it's dependency, JSON.jl) to avoid participating in the package environment altogether. But that seems a bit extreme...

That's not a bad idea.

[ericphanson]

Ok, I pushed a commit to use a separate environment for GitHubActions.jl. Now I'll try and test this (ericphanson/VisualStringDistances.jl#16).

[ericphanson]

It works! See https://github.com/ericphanson/VisualStringDistances.jl/pull/16/files to see the beautiful annotation (press a if you don't see it), and https://github.com/ericphanson/VisualStringDistances.jl/runs/3726060940?check_suite_focus=true#step:5:14 to confirm we are using our own environment.

[ericphanson]

oh, I should call out the --code-coverage flag is a separate addition here. I can split that into a separate PR if desired. The reason I think it's really useful is that it means you can get coverage from doctests just running the process-coverage action after.

[SaschaMann]

A separate PR or two separate commits would be nice so that we can link to them in the release notes.

[ericphanson]

Ok, I took it out here so this one can be squash-merged and then I’ll make a separate PR to add it

[ericphanson]

Arg, I wasn't testing on julia 1.0 like I thought I was (because I wasn't quoting the version number like the docs say to do). In particular I don't think Pkg.add(; shared=true) is supported (https://github.com/ericphanson/VisualStringDistances.jl/runs/3726570300?check_suite_focus=true). Hm...

[ericphanson]

Things seem to be working on 1.0 (https://github.com/ericphanson/VisualStringDistances.jl/runs/3726621915), and with @fredrikekre's suggestions, our addition of the logger is pretty well isolated from the docs script, so this is good to go from my end.

[SaschaMann]

Tagged a release: https://github.com/julia-actions/julia-docdeploy/releases/tag/v1.2.0

Thanks everyone :)

[ericphanson]

We are getting close to fulfulling all my doctest ambitions 😄 https://github.com/users/ericphanson/projects/1