Add package extension for Makie

[sloede]

This PR creates a package extension for the additional functionality we implemented that requires Makie.jl to be loaded. To remain backwards compatible with Julia v1.8, I've kept the current approach that is based on Requires.jl.

Package extensions were added in Julia v1.9. However, in the original implementation they are not triggered if the dependency is loaded from a different environment than the currently active one. This was remedied only in JuliaLang/julia#49701 (and will end up in Julia v1.9.1), thus I created the version guard such that the referenced PR is included.

When an appropriate, new Julia version is used, the package extension is loaded instead of relying on Requires.jl. The only user-visible difference is that we need to define and export the functions iplot and iplot! already in Trixi.jl, since the package extension cannot export anything itself. This is somewhat against our usual policy and (and against POLA), since it means we have an exported function that has no usable implementation unless Makie.jl is loaded. @KristofferC suggested a potential remedy on Slack:

For the plotting case why not just export whatever name of the plotting function you want in the parent package, put the code for plotting in the extension and then call that code from the parent plotting function based on checking Base.get_extension when it is called. If the extension is not loaded, give some informative error message?

However, for now I do not think this is necessary, as it creates new headaches (e.g., it splits the API definition between two places). Thus I suggest to do it this way for now and gather some real-world experience before using a more complicated implementation.

[codecov]

Codecov Report

Merging #1494 (f8b32aa) into main (da457e4) will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##             main    #1494   +/-   ##
=======================================
  Coverage   95.40%   95.40%           
=======================================
  Files         360      360           
  Lines       29926    29925    -1     
=======================================
  Hits        28548    28548           
+ Misses       1378     1377    -1     

Flag	Coverage Δ
unittests	95.40% <33.33%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
ext/TrixiMakieExt.jl	97.02% <ø> (ø)
src/Trixi.jl	71.43% <33.33%> (+3.25%)	⬆️

[ranocha]

For reference: We assume the fix for package extensions will be available in Julia v1.9.1 since it's included in the corresponding backports PR JuliaLang/julia#49680

[ranocha]

Thanks a lot for initiating this!

[ranocha]

Did you test this with

• some older version of Julia using the Requires.jl branch
• some development version of Trixi.jl using the package extensions branch

?

[sloede]

Did you test this with

* some older version of Julia using the Requires.jl branch

* some development version of Trixi.jl using the package extensions branch

?

With my original implementation, I tried Julia v1.9.0 and v1.10.0-DEV.1288. They both worked and showed the expected behavior. For v1.8, I thought to let CI sort it out whether it works or not

[KristofferC]

If Requires is only used for this feature (I didn't check) it could also be conditionally loaded.

[sloede]

If Requires is only used for this feature (I didn't check) it could also be conditionally loaded.

No, it's still used for Plots.jl and some other stuff, but we'll eventually migrate everything. This is just to get our feet wet and to resolve the precompilation issue you mentioned in #ttfx

[ranocha]

I assume you copy-pasted everything correctly - didn't check that

[ranocha]

Thanks!

[KristofferC]

However, for now I do not think this is necessary, as it creates new headaches (e.g., it splits the API definition between two places).

While true, I think a user is more happy to get an informative error message over

julia> iplot(rand(10))
ERROR: MethodError: no method matching iplot(::Vector{Float64})

[sloede]

While true, I think a user is more happy to get an informative error message

I completely agree. But how would you implement that without having to duplicate the function signature from the extension inside the main package? And adding a catchall iplot(args...; kwargs...) in the main package that throws an error also seems like a non optimal (and potentially confusing) solution.

[sloede]

@ranocha I added the ext directory to the list of folders that get processed for coverage. I didn't find anything else in our various workflow files, but could you please have a look as well that I didn't miss anything CI related?

[ranocha]

I think it should be fine but we can never be sure as long as the Linux MPI tests keep failing. I created #1497 to mitigate this a bit. If you agree, we could first consider merging that PR to see whether everything is fine with coverage etc. here

[sloede]

If you agree, we could first consider merging that PR to see whether everything is fine with coverage etc. here

Sounds good!

[ranocha]

Coverage looks good. Thanks, @sloede