RFC: test, fix, and redesign Requires

[timholy]

This is a very intrusive change that also has the consequence of getting this package working on 0.7. The first commit restructures the tests to be more typical, in my opinion, of how I suspect people use Requires in practice. It tests both non-precompiled and precompiled packages, and tests behavior from outside the module that leverages Requires to ensure that there's no weird scoping issue. If you check out just this commit, you can easily verify that it fails these new tests for precompiled packages.

The second commit is the dangerous one. One change you definitely want to keep is hard-coding the PkgId that gets passed to listenpkg:

https://github.com/MikeInnes/Requires.jl/blob/dc9c472f0494ee2a858a13ebffeb0afc03e461a0/src/require.jl#L64

Precompilation, for reasons I don't fully understand, turns the $pkg key into JSON [top-level], which is what populates Requires._callbacks. Consequently, when the package callback comes in with a key JSON [682c06a0-de6a-54ab-a142-c8b1cf79cde6], it doesn't find it in the dictionary and Requires silently fails to load the requested resource.

However, this wasn't (I think---I could have made a user error while putting this together) enough to fix the package completely. I have to confess I wasn't too fond of the way that Requires constructed an __init__ function, especially since my reading of the code suggests that if a user needs to have her own __init__ doing something else, Requires will silently fail. Consequently I took the daring step of also redesigning how one should use Requires---now you do this:

module Mine

using Requires

#= code =#

function __init__()
    #= any specific initialization that this package needs =#
    @require pkg1=uuid1 expr1
    @require pkg2=uuid2 expr2
    ...
end

end

This leads to less need for macro-foo, and might make it easier for people to understand how this works and consequently help maintain it (e.g., #45). In my opinion it's also less magical-seeming and may help people have a bit more intuition for how this works.

[timholy]

CC @Datseris @tpapp

[tpapp]

Thanks for making this PR. I was stuck with #45 because the mechanism for @require was too opaque for me, this is easier to understand. Having the code in __init__ makes it explicit that some kind of handler is registered.

[Datseris]

Thanks for making this PR, it will be amazing to get this working for 0.7, I already use it for 2 out of the 4 packages I have created :D

The new interface, e.g. using __init__() is totally fine. Whether or not it makes the usage more clear, it depends on the level of the user. For me, that is not really much of an expert, it is just as clear as the previous version and thus the change is totally fine. For users like tpapp which have more knowledge, it seems to make everything even clearer.

Another plus is that this approach does not add any extra lines of code to users.
You either do

begin 
   @require
end

or

function __init__()
    @require
end

same story length-wise. The change does have the huge benefit that Tim mentioned, that it does not silently fail if an init is already defined.

[timholy]

There is now a mini-blog post on this here (written by request in the comment above it).

[vtjnash]

Sounds good to me. Might be useful to encourage pre-compilation compatibility and discourage changing local bindings though. Those together mean that it would encourage providing just a mapping of one module name name to another.

when_loaded(@__MODULE__, :JSON, :MyJsonPlugin)
# or
@when_loaded JSON => MyJsonPlugin

This would records locally that when and if JSON is loaded, then MyJsonPlugin should also get loaded.

Requires.jl can implement this by creating a global array inside the current-module recording these relations in a hidden magic variable. Then when Requires.jl gets the module-load-callback, it can scan through the list of all modules and look for this hidden metadata and prepare a list of additional actions to perform.

[vtjnash]

Precompilation, for reasons I don't fully understand, turns the $pkg key into JSON [top-level]

Create a fully-qualified token with PkgId(__module__, :symbol) to pass to Base.require to lookup the Symbol in the namespace for the module provided by the package-manager. (and/or do the comparison against Symbol(ThePkgId.name))

[MikeInnes]

Great stuff @timholy! This package has been fairly minimally maintained since the first release (I think for 0.4?) so it's no doubt due for an overhaul.

FWIW, it is possible to have @init and your own initialisation play nice together, but obviously it's not documented and I agree that the implicitness isn't great. I'm fine with this API but would still like to do @init @require ... in places just to keep things cleanly separated, so for now I propose giving @init a home here even though it's not used directly.

It's a shame we can't easily deprecate this (?) but luckily we can at least re-use the syntax deprecation to warn people.

[davidanthoff]

Thanks everyone for sorting this out, much appreciated!!

[timholy]

Thanks Mike, I really appreciate the work you've done yourself putting this on more solid footing. And thanks for adding the syntax warning, that should cover people who haven't yet tried to update to 0.7.

@vtjnash, I too wondered about the const binding for the module name. Seems worth playing with. And with regards to the token, yes, that's exactly how I resolved it.

[tlnagy]

I'm currently moving Compose to using Requires and I would like to use this new approach for loading code, but I don't know where I can find the UUIDs? A lot of packages don't have a Project.toml files etc yet?

EDIT: Looks like the same question was asked in #51

[MikeInnes]

#51

[davidavdav]

Sorry, I don't understand.

So how do I specify a bunch of code to load from my package Mymodule when the user uses LinearAlgebra after having loaded Mymodule?

using Mymodule
using LinearAlgebra
## now Mymodule's support for functions in LinearAlgebra is loaded