Refactor: do not use double constructor for PackageSpec

[00vareladavid]

Fix #2282

I admit that messing with the constructor was a thoroughly terrible idea. The check could be circumvented anyway by manually setting the fields after the constructor call. The checking should be done inside the API.

[KristofferC]

Could the handle_package_input! be added around here instead

Pkg.jl/src/API.jl

Line 146 in 9cf608c

ret = $f(ctx, pkgs; kwargs...)

? And then it is used for everything?

[00vareladavid]

There is something weird going on with the artifacts tests. It was giving some bad input to Pkg.add. I adjusted them to what I thought was the most likely intention, but I do not really understand those tests yet.

[00vareladavid]

Perhaps @staticfloat (or anyone who knows those tests) could take a quick look. I'm almost certain it was just a matter of git initing the test packages, but there was a mismatch between some UUIDs that I'm not so sure about.

[staticfloat]

Why did you need to change the UUID here?

[00vareladavid]

I changed it to the value of aol_uuid because that was the value explicitly given here:

Pkg.jl/test/artifacts.jl

Line 641 in f073140

uuid=aol_uuid,

. If they are not the same value, Pkg will error out.

[staticfloat]

huh, why wasn't this erroring before?

[00vareladavid]

There is a misuse of the API here. API calls were supposed to be made with Pkg.PackageSpec, not Pkg.Types.PackageSpec. This is needlessly confusing, which is why this PR removes the second constructor.

[staticfloat]

Right, I'm specifically interested in why this having the wrong UUID didn't cause an issue before.

[00vareladavid]

Because Pkg.PackageSpec did some pre-processing, which was circumvented by using Pkg.Types.PackageSpec. In particular, Pkg.PackageSpec automatically sets repo.source, while Pkg.Types.PackageSpec does not. So the package was in some undefined state which just happened to appear like it was tracking a path when it was written to the manifest.

[00vareladavid]

So it seems like the intention was that they were supposed to be the same UUID? That is the only thing I was not sure of.

[staticfloat]

Yes, they definitely should have matched

[00vareladavid]

Ok, thanks. I think this slight adjustment was all that was necessary

[staticfloat]

What were the errors you were seeing? I'm not sure why git_init_package() would have any different behavior here from copy_test_package()?

[00vareladavid]

Thanks for reviewing. Pkg.add requires the package to be git version controlled because it always clones (it doesn't track the path directly like Pkg.develop does).

[DilumAluthge]

It looks like this PR made a breaking change in the public API:

On Julia 1.6.1:

julia> import Pkg

julia> Pkg.PackageSpec(; name = "Example", uuid = "7876af07-990d-54b4-ab0e-23690620f79a")
PackageSpec(
 name = Example
 uuid = 7876af07-990d-54b4-ab0e-23690620f79a
 version = *
)

On Julia master:

julia> import Pkg

julia> Pkg.PackageSpec(; name = "Example", uuid = "7876af07-990d-54b4-ab0e-23690620f79a")
ERROR: MethodError: Cannot `convert` an object of type String to an object of type Base.UUID
Closest candidates are:
 convert(::Type{T}, ::T) where T at essentials.jl:228
 Base.UUID(::AbstractString) at uuid.jl:87
 Base.UUID(::Any) at uuid.jl:9
Stacktrace:
 [1] convert(#unused#::Type{Union{Nothing, Base.UUID}}, x::String)
  @ Base ./some.jl:36
 [2] Pkg.Types.PackageSpec(name::String, uuid::String, version::Pkg.Versions.VersionSpec, tree_hash::Nothing, repo::Pkg.Types.GitRepo, path::Nothing, pinned::Bool, url::Nothing, rev::Nothing, subdir::Nothing)
  @ Pkg.Types ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Pkg/src/Types.jl:92
 [3] Pkg.Types.PackageSpec(; name::String, uuid::String, version::Pkg.Versions.VersionSpec, tree_hash::Nothing, repo::Pkg.Types.GitRepo, path::Nothing, pinned::Bool, url::Nothing, rev::Nothing, subdir::Nothing)
  @ Pkg.Types ./util.jl:471
 [4] top-level scope
  @ REPL[1]:1

Is this easy to fix? Or should we revert this PR?