yanked packages cause resolver problems when testing

[simonbyrne]

If a yanked version is used in the Manifest.toml, then Pkg.test will fail when it attempts to resolve test dependencies:

+ ./julia-1.1/bin/julia --project=@. -e 'using Pkg; Pkg.test()'
   Testing CLIMA
 Resolving package versions...
ERROR: Unsatisfiable requirements detected for package BinaryProvider [b99e7846]:
 BinaryProvider [b99e7846] log:
 ├─possible versions are: [0.1.0-0.1.5, 0.2.2-0.2.3, 0.2.5-0.2.8, 0.3.0, 0.3.2-0.3.3, 0.4.0-0.4.2, 0.5.0-0.5.4, 0.5.6] or uninstalled
 └─restricted to versions 0.5.5 by an explicit requirement — no versions left
Stacktrace:
 [1] check_constraints(::Pkg.GraphType.Graph) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/GraphType.jl:937
 [2] Pkg.GraphType.Graph(::Dict{Base.UUID,Set{VersionNumber}}, ::Dict{Base.UUID,Dict{Pkg.Types.VersionRange,Dict{String,Base.UUID}}}, ::Dict{Base.UUID,Dict{Pkg.Types.VersionRange,Dict{String,Pkg.Types.VersionSpec}}}, ::Dict{Base.UUID,String}, ::Dict{Base.UUID,Pkg.Types.VersionSpec}, ::Dict{Base.UUID,Pkg.Types.Fixed}, ::Bool) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/GraphType.jl:364
 [3] deps_graph(::Pkg.Types.Context, ::Dict{Base.UUID,String}, ::Dict{Base.UUID,Pkg.Types.VersionSpec}, ::Dict{Base.UUID,Pkg.Types.Fixed}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/Operations.jl:306
 [4] resolve_versions!(::Pkg.Types.Context, ::Array{Pkg.Types.PackageSpec,1}, ::Nothing) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/Operations.jl:369
 [5] resolve_versions! at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/Operations.jl:315 [inlined]
 [6] #add_or_develop#63(::Array{Base.UUID,1}, ::Symbol, ::Function, ::Pkg.Types.Context, ::Array{Pkg.Types.PackageSpec,1}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/Operations.jl:1172
 [7] add_or_develop at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/Operations.jl:1156 [inlined]
 [8] (::getfield(Pkg.Operations, Symbol("##40#44")){Bool,getfield(Pkg.Operations, Symbol("##68#70")){Pkg.Types.Context,getfield(Pkg.Operations, Symbol("##67#69")){Pkg.Types.Context,Cmd}},Pkg.Types.Context,Pkg.Types.PackageSpec,Pkg.Types.Context})(::String) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/Operations.jl:874
 [9] mktempdir(::getfield(Pkg.Operations, Symbol("##40#44")){Bool,getfield(Pkg.Operations, Symbol("##68#70")){Pkg.Types.Context,getfield(Pkg.Operations, Symbol("##67#69")){Pkg.Types.Context,Cmd}},Pkg.Types.Context,Pkg.Types.PackageSpec,Pkg.Types.Context}, ::String) at ./file.jl:581
 [10] mktempdir at ./file.jl:579 [inlined]
 [11] #with_dependencies_loadable_at_toplevel#38(::Bool, ::Function, ::getfield(Pkg.Operations, Symbol("##68#70")){Pkg.Types.Context,getfield(Pkg.Operations, Symbol("##67#69")){Pkg.Types.Context,Cmd}}, ::Pkg.Types.Context, ::Pkg.Types.PackageSpec) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/Operations.jl:853
 [12] #with_dependencies_loadable_at_toplevel at ./none:0 [inlined]
 [13] #test#66(::Bool, ::Function, ::Pkg.Types.Context, ::Array{Pkg.Types.PackageSpec,1}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/Operations.jl:1319
 [14] #test at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/API.jl:0 [inlined]
 [15] #test#46(::Bool, ::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}, ::Function, ::Pkg.Types.Context, ::Array{Pkg.Types.PackageSpec,1}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/API.jl:198
 [16] test at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/API.jl:183 [inlined]
 [17] #test#45 at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/API.jl:180 [inlined]
 [18] test at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/API.jl:180 [inlined]
 [19] #test#42 at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/API.jl:177 [inlined]
 [20] test() at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Pkg/src/API.jl:177
 [21] top-level scope at none:0

(from https://dev.azure.com/spjbyrne/CLIMA/_build/results?buildId=150)

If yanking is to be used sparingly, I would be okay with failure here (though would prefer a more informative error message).

[IanButterworth]

Given the frequency in which I've seen CI on a checked-in manifest break during Pkg.test like this when a package is yanked, I think we might want to instead of error in the resolver put Pkg into a mode where it is allowed to resolve yanked versions but with a very loud warning.

If yanking was only done for security reasons then I think erroring like above is reasonable, but arguably Pkg.instantiate should also error, which it doesn't.

However from what I've seen yanking is most commonly used for tricky compat issues (that I believe there is usually a way around, but that's another discussion).

So above might look like

% julia --project=@. -e 'using Pkg; Pkg.test()'
   Testing Foo
 Resolving package versions...
-------
WARNING

The package manifest has a dependency that has been yanked.
Pkg.test will use this version and respect the manifest, but it is recommended to investigate the yanking and fix the dependency version in manifest {manifest_path}

Yanked: Bar v1.2.3

-----
{The rest of Pkg.status output etc.}

[DilumAluthge]

Hmmmm, we'd only want to do this in a situation where the user checked their manifest into source control, right? Which is not most cases.

Should we instead have an allow_yanked_packages kwarg to Pkg.test, defaulting to false? Then, on repos where the user checks the manifest into source control, the user could also just specify allow_yanked_packages=true?

[IanButterworth]

That sounds good.

I do think we should also add a warning in that case if a yanked package is installed.

And the same warning for instantiate, which already installs yanked packages.

[Octogonapus]

I would be fine with an allow_yanks_packages kwarg for Pkg.test (which defaults to false), though I think users should avoid that as much as possible. Yanks can be security issues and I would not want that running in my CI. I also think Pkg.instantiate should be made consistent and not install yanked packages (it could also get this kwarg).

Indeed this frustration with yanks comes from too many yanks, and I would support working on those root causes rather than just ignoring yanks and losing that utility. As far as I understand, those root causes are all outside this repository (downgrade CI, yank culture, etc.) and should be tackled separately.