Make Pkg.test check for ambiguities

[timholy]

Reference JuliaLang/julia#17418 (comment). Someone who knows more about the package machinery than me should comment on whether it's safe to assume there's a module with the same name as the package.

[tkelman]

Okay looking at Pkg.test more closely it doesn't actually do using or anything involving the module, it just executes test/runtests.jl if that exists. A few options then, of which we could do any combination:

1. Do the ambiguity-detection in a second process as part of Pkg.test, maybe with a default setting of "warn on ambiguities and exit non-fatally if something goes wrong" - with keyword arguments to either skip the extra ambiguity part, or make it fatal and throw errors
2. Add detect_ambiguities(Base, name) to the default test/runtests.jl script in PkgDev - will only benefit new packages right away, unless we make a PR spree (which I'm happy to write a script to do automatically btw)
3. Check for it (either report it as a failure or report a new "has ambiguities" status code?) on PkgEval
4. Add the checking to the default Travis script (which is defined at https://github.com/travis-ci/travis-build/blob/master/lib/travis/build/script/julia.rb) for 0.5 and newer - downside here is packages that can't use travis, or choose to overwrite the default script setting won't get this automatically

other ideas?

[timholy]

With regards to identifying modules loaded during testing, it seems that this will work:

nms1 = names(Main)
include("runtests.jl")
nms2 = names(Main)
mods = filter(x->isa(getfield(Main, x), Module), setdiff(nms2, nms1))

I don't think there's any real harm in extending the list beyond the main module: the idea is that a package that has dependencies shouldn't conflict with its dependencies. The only exception would be things loaded only for testing, but hopefully that will rarely be a problem.

I would favor:

• adding this to Pkg.test, defaulting to a warning
• on Travis, this defaults to an error (people only look at Travis logs if something fails)

There's always something a little annoying about having local test results differ from Travis, of course.

[aviks]

I would urge not having different results between local tests and Travis. I'll predict Two years later, when everyone involved has forgotten about this issue, it will become a major point of confusion among newcomers.

For long term support and sanity, I would therefore much prefer option 2 above, with the option set to "fail on ambiguities"

[timholy]

Option 2 works for me, esp. if @tkelman is willing to write that script 😄.

[tkelman]

what do you think it should look like, exactly? as I had it above, detect_ambiguities(pkgmodule) or also checking against Base?

[timholy]

Good idea to include Base in the list.

[tkelman]

So what exactly is your suggestion? Add isdefined(Base.Test, :detect_ambiguities) && Base.Test.detect_ambiguities(Base, pkgmodule) at the start or end of test/runtests.jl ? Once 0.5.0-rc0 binaries are built I plan on submitting an automated set of PR's to nearly all registered packages to add 0.5 to their .travis.yml file (and change release to 0.4, if present), and I could do this appending via the same set of scripts, either within the same PR per package or separately (separately probably makes more sense but would also cost more against my github api rate limit).

[timholy]

Obviously this is worth getting right since it will be a pain to do it over. Definitely recommend a few trial balloons first.

I think it will be correct if we detect the loaded modules like in https://github.com/JuliaLang/julia/issues/17444#issuecomment-233400881, and then use Base.Test.detect_ambiguities(Base, mods...). That seems likely to be conservative (it will catch ambiguities between a package and any packages that are used only for testing), but as long as it's issuing a warning rather than an error I think that's OK as default behavior.

[tkelman]

Definitely recommend a few trial balloons first.

I'll put if author == "timholy" in the first version of the script then :)

What about imported = true or should it stay false?

as long as it's issuing a warning rather than an error

Is that the default behavior of detect_ambiguities ?

[timholy]

detect_ambiguities just returns the list of pairwise ambiguities; you get to decide what to do when it's not empty.

Presumably imported=true would again be the most conservative.

[KristofferC]

I don't think this should be up to the Package manager but rather the test system. It is "cleanest" if the package manager basically just calls runtests.jl.

[timholy]

Agreed, this is definitely not a Pkg.jl issue.