Refactor: do not use double constructor for PackageSpec (#2477)

* Refactor: do not use double constructor for PackageSpec
* adjust artifact tests

src/API.jl

@@ -143,6 +143,8 @@ for f in (:develop, :add, :rm, :up, :pin, :free, :test, :build, :status)
             Registry.download_default_registries(io)
             ctx = Context()
             kwargs = merge((;kwargs...), (:io => io,))
+            pkgs = deepcopy(pkgs) # don't mutate input
+            foreach(pkg -> handle_package_input!(pkg), pkgs)
             ret = $f(ctx, pkgs; kwargs...)
             $(f in (:add, :up, :pin, :free, :build)) && Pkg._auto_precompile(ctx)
             return ret
@@ -151,35 +153,33 @@ for f in (:develop, :add, :rm, :up, :pin, :free, :test, :build, :status)
         function $f(; name::Union{Nothing,AbstractString}=nothing, uuid::Union{Nothing,String,UUID}=nothing,
                       version::Union{VersionNumber, String, VersionSpec, Nothing}=nothing,
                       url=nothing, rev=nothing, path=nothing, mode=PKGMODE_PROJECT, subdir=nothing, kwargs...)
-            pkg = Package(name=name, uuid=uuid, version=version, url=url, rev=rev, path=path, subdir=subdir)
+            pkg = PackageSpec(; name=name, uuid=uuid, version=version, url=url, rev=rev, path=path, subdir=subdir)
             if $f === status || $f === rm || $f === up
                 kwargs = merge((;kwargs...), (:mode => mode,))
             end
             # Handle $f() case
-            if pkg == Package()
+            if unique([name,uuid,version,url,rev,path,subdir]) == [nothing]
                 $f(PackageSpec[]; kwargs...)
             else
                 $f(pkg; kwargs...)
             end
         end
         function $f(pkgs::Vector{<:NamedTuple}; kwargs...)
-            $f([Package(;pkg...) for pkg in pkgs]; kwargs...)
+            $f([PackageSpec(;pkg...) for pkg in pkgs]; kwargs...)
         end
     end
 end
 
 function develop(ctx::Context, pkgs::Vector{PackageSpec}; shared::Bool=true,
                  preserve::PreserveLevel=PRESERVE_TIERED, platform::AbstractPlatform=HostPlatform(), kwargs...)
     require_not_empty(pkgs, :develop)
-    foreach(pkg -> check_package_name(pkg.name, :develop), pkgs)
-    pkgs = deepcopy(pkgs) # deepcopy for avoid mutating PackageSpec members
     Context!(ctx; kwargs...)
 
     for pkg in pkgs
+        check_package_name(pkg.name, "develop")
         if pkg.name == "julia" # if julia is passed as a package the solver gets tricked
             pkgerror("`julia` is not a valid package name")
         end
-        pkg.name === nothing || check_package_name(pkg.name, "develop")
         if pkg.name === nothing && pkg.uuid === nothing && pkg.repo.source === nothing
             pkgerror("name, UUID, URL, or filesystem path specification required when calling `develop`")
         end
@@ -217,18 +217,16 @@ end
 function add(ctx::Context, pkgs::Vector{PackageSpec}; preserve::PreserveLevel=PRESERVE_TIERED,
              platform::AbstractPlatform=HostPlatform(), kwargs...)
     require_not_empty(pkgs, :add)
-    foreach(pkg -> check_package_name(pkg.name, :add), pkgs)
-    pkgs = deepcopy(pkgs)  # deepcopy for avoid mutating PackageSpec members
     Context!(ctx; kwargs...)
 
     for pkg in pkgs
+        check_package_name(pkg.name, "add")
         if pkg.name == "julia" # if julia is passed as a package the solver gets tricked
             pkgerror("`julia` is not a valid package name")
         end
         if pkg.name === nothing && pkg.uuid === nothing && pkg.repo.source === nothing
             pkgerror("name, UUID, URL, or filesystem path specification required when calling `add`")
         end
-        pkg.name === nothing || check_package_name(pkg.name, "add")
         if pkg.repo.source !== nothing || pkg.repo.rev !== nothing
             if pkg.version != VersionSpec()
                 pkgerror("version specification invalid when tracking a repository:",
@@ -270,12 +268,12 @@ function add(ctx::Context, pkgs::Vector{PackageSpec}; preserve::PreserveLevel=PR
 end
 
 function rm(ctx::Context, pkgs::Vector{PackageSpec}; mode=PKGMODE_PROJECT, all_pkgs::Bool=false, kwargs...)
+    Context!(ctx; kwargs...)
     if all_pkgs
         !isempty(pkgs) && pkgerror("cannot specify packages when operating on all packages")
         append_all_pkgs!(pkgs, ctx, mode)
     end
     require_not_empty(pkgs, :rm)
-    pkgs = deepcopy(pkgs)  # deepcopy for avoid mutating PackageSpec members
 
     for pkg in pkgs
         if pkg.name === nothing && pkg.uuid === nothing
@@ -288,8 +286,6 @@ function rm(ctx::Context, pkgs::Vector{PackageSpec}; mode=PKGMODE_PROJECT, all_p
         end
     end
 
-    Context!(ctx; kwargs...)
-
     mode == PKGMODE_PROJECT && project_deps_resolve!(ctx.env, pkgs)
     mode == PKGMODE_MANIFEST && manifest_resolve!(ctx.env.manifest, pkgs)
     ensure_resolved(ctx.env.manifest, pkgs)
@@ -315,8 +311,6 @@ end
 function up(ctx::Context, pkgs::Vector{PackageSpec};
             level::UpgradeLevel=UPLEVEL_MAJOR, mode::PackageMode=PKGMODE_PROJECT,
             update_registry::Bool=true, kwargs...)
-    pkgs = deepcopy(pkgs)  # deepcopy for avoid mutating PackageSpec members
-
     Context!(ctx; kwargs...)
     if update_registry
         Registry.download_default_registries(ctx.io)
@@ -344,13 +338,12 @@ function resolve(ctx::Context; kwargs...)
 end
 
 function pin(ctx::Context, pkgs::Vector{PackageSpec}; all_pkgs::Bool=false, kwargs...)
+    Context!(ctx; kwargs...)
     if all_pkgs
         !isempty(pkgs) && pkgerror("cannot specify packages when operating on all packages")
         append_all_pkgs!(pkgs, ctx, PKGMODE_PROJECT)
     end
     require_not_empty(pkgs, :pin)
-    pkgs = deepcopy(pkgs)  # deepcopy for avoid mutating PackageSpec members
-    Context!(ctx; kwargs...)
 
     for pkg in pkgs
         if pkg.name === nothing && pkg.uuid === nothing
@@ -376,13 +369,12 @@ function pin(ctx::Context, pkgs::Vector{PackageSpec}; all_pkgs::Bool=false, kwar
 end
 
 function free(ctx::Context, pkgs::Vector{PackageSpec}; all_pkgs::Bool=false, kwargs...)
+    Context!(ctx; kwargs...)
     if all_pkgs
         !isempty(pkgs) && pkgerror("cannot specify packages when operating on all packages")
         append_all_pkgs!(pkgs, ctx, PKGMODE_PROJECT)
     end
     require_not_empty(pkgs, :free)
-    pkgs = deepcopy(pkgs)  # deepcopy for avoid mutating PackageSpec members
-    Context!(ctx; kwargs...)
 
     for pkg in pkgs
         if pkg.name === nothing && pkg.uuid === nothing
@@ -411,8 +403,8 @@ function test(ctx::Context, pkgs::Vector{PackageSpec};
               kwargs...)
     julia_args = Cmd(julia_args)
     test_args = Cmd(test_args)
-    pkgs = deepcopy(pkgs) # deepcopy for avoid mutating PackageSpec members
     Context!(ctx; kwargs...)
+
     if isempty(pkgs)
         ctx.env.pkg === nothing && pkgerror("trying to test unnamed project") #TODO Allow this?
         push!(pkgs, ctx.env.pkg)
@@ -972,7 +964,6 @@ function gc(ctx::Context=Context(); collect_delay::Period=Day(7), verbose=false,
 end
 
 function build(ctx::Context, pkgs::Vector{PackageSpec}; verbose=false, kwargs...)
-    pkgs = deepcopy(pkgs)  # deepcopy for avoid mutating PackageSpec members
     Context!(ctx; kwargs...)
 
     if isempty(pkgs)
@@ -1654,23 +1645,16 @@ end
 
 @deprecate setprotocol!(proto::Union{Nothing, AbstractString}) setprotocol!(protocol = proto) false
 
-# API constructor
-function Package(;name::Union{Nothing,AbstractString} = nothing,
-                 uuid::Union{Nothing,String,UUID} = nothing,
-                 version::Union{VersionNumber, String, VersionSpec, Nothing} = nothing,
-                 url = nothing, rev = nothing, path=nothing,
-                 subdir = nothing)
-    if path !== nothing && url !== nothing
+function handle_package_input!(pkg::PackageSpec)
+    if pkg.path !== nothing && pkg.url !== nothing
         pkgerror("`path` and `url` are conflicting specifications")
     end
-    repo = Types.GitRepo(rev = rev, source = url !== nothing ? url : path, subdir = subdir)
-    version = version === nothing ? VersionSpec() : VersionSpec(version)
-    uuid = uuid isa String ? UUID(uuid) : uuid
-    PackageSpec(;name=name, uuid=uuid, version=version, path=nothing,
-                repo=repo, tree_hash=nothing)
+    pkg.repo = Types.GitRepo(rev = pkg.rev, source = pkg.url !== nothing ? pkg.url : pkg.path,
+                         subdir = pkg.subdir)
+    pkg.path = nothing
+    pkg.tree_hash = nothing
+    pkg.version = pkg.version === nothing ? VersionSpec() : VersionSpec(pkg.version)
+    pkg.uuid = pkg.uuid isa String ? UUID(pkg.uuid) : pkg.uuid
 end
-Package(name::AbstractString) = PackageSpec(name)
-Package(name::AbstractString, uuid::UUID) = PackageSpec(name, uuid)
-Package(name::AbstractString, uuid::UUID, version::VersionTypes) = PackageSpec(name, uuid, version)
 
 end # module

src/Pkg.jl

@@ -496,7 +496,7 @@ Below is a comparison between the REPL mode and the functional API:
 | `--major Package`    | `PackageSpec(name="Package", version=PKGLEVEL_MAJOR)` |
 
 """
-const PackageSpec = API.Package
+const PackageSpec = Types.PackageSpec
 
 """
     setprotocol!(;

src/REPLMode/argument_parsers.jl

@@ -11,6 +11,10 @@ function parse_package(args::Vector{QString}, options; add_or_dev=false)::Vector
     return parse_package_args(args; add_or_dev=add_or_dev)
 end
 
+struct VersionToken
+    version::String
+end
+
 struct Rev
     rev::String
 end
@@ -20,7 +24,7 @@ struct Subdir
 end
 
 const PackageIdentifier = String
-const PackageToken = Union{PackageIdentifier, VersionRange, Rev, Subdir}
+const PackageToken = Union{PackageIdentifier, VersionToken, Rev, Subdir}
 
     # Match a git repository URL. This includes uses of `@` and `:` but
     # requires that it has `.git` at the end.
@@ -64,7 +68,7 @@ function package_lex(qwords::Vector{QString})::Vector{String}
 end
 
 PackageToken(word::String)::PackageToken =
-    first(word) == '@' ? VersionRange(word[2:end]) :
+    first(word) == '@' ? VersionToken(word[2:end]) :
     first(word) == '#' ? Rev(word[2:end]) :
     first(word) == ':' ? Subdir(word[2:end]) :
     String(word)
@@ -75,15 +79,15 @@ function parse_package_args(args::Vector{PackageToken}; add_or_dev=false)::Vecto
         (isempty(args) || args[1] isa PackageIdentifier) && return
         modifier = popfirst!(args)
         if modifier isa Subdir
-            pkg.repo.subdir = modifier.dir
+            pkg.subdir = modifier.dir
             (isempty(args) || args[1] isa PackageIdentifier) && return
             modifier = popfirst!(args)
         end
 
-        if modifier isa VersionRange
-            pkg.version = VersionSpec(modifier)
+        if modifier isa VersionToken
+            pkg.version = modifier.version
         elseif modifier isa Rev
-            pkg.repo.rev = modifier.rev
+            pkg.rev = modifier.rev
         else
             pkgerror("Package name/uuid must precede subdir specifier `[$arg]`.")
         end
@@ -98,7 +102,7 @@ function parse_package_args(args::Vector{PackageToken}; add_or_dev=false)::Vecto
             push!(pkgs, pkg)
         # Modifiers without a corresponding package identifier -- this is a user error
         else
-            arg isa VersionRange ?
+            arg isa VersionToken ?
                 pkgerror("Package name/uuid must precede version specifier `@$arg`.") :
             arg isa Rev ?
                 pkgerror("Package name/uuid must precede revision specifier `#$(arg.rev)`.") :
@@ -119,10 +123,10 @@ end
 function parse_package_identifier(word::AbstractString; add_or_develop=false)::PackageSpec
     if add_or_develop
         if isurl(word)
-            return PackageSpec(repo=Types.GitRepo(source=word))
+            return PackageSpec(; url=word)
         elseif any(occursin.(['\\','/'], word)) || word == "." || word == ".."
             if casesensitive_isdir(expanduser(word))
-                return PackageSpec(repo=Types.GitRepo(source=normpath(expanduser(word))))
+                return PackageSpec(; path=normpath(expanduser(word)))
             else
                 pkgerror("`$word` appears to be a local path, but directory does not exist")
             end
@@ -132,7 +136,7 @@ function parse_package_identifier(word::AbstractString; add_or_develop=false)::P
         end
     end
     if occursin(uuid_re, word)
-        return PackageSpec(uuid=UUID(word))
+        return PackageSpec(;uuid=UUID(word))
     elseif occursin(name_re, word)
         return PackageSpec(String(match(name_re, word).captures[1]))
     elseif occursin(name_uuid_re, word)

src/Types.jl

@@ -91,11 +91,15 @@ Base.:(==)(r1::GitRepo, r2::GitRepo) =
 Base.@kwdef mutable struct PackageSpec
     name::Union{Nothing,String} = nothing
     uuid::Union{Nothing,UUID} = nothing
-    version::VersionTypes = VersionSpec()
+    version::Union{Nothing,VersionTypes,String} = VersionSpec()
     tree_hash::Union{Nothing,SHA1} = nothing
     repo::GitRepo = GitRepo()
     path::Union{Nothing,String} = nothing
     pinned::Bool = false
+    # used for input only
+    url = nothing
+    rev = nothing
+    subdir = nothing
 end
 PackageSpec(name::AbstractString) = PackageSpec(;name=name)
 PackageSpec(name::AbstractString, uuid::UUID) = PackageSpec(;name=name, uuid=uuid)
@@ -126,17 +130,20 @@ function Base.show(io::IO, pkg::PackageSpec)
     pkg.name !== nothing && push!(f, "name" => pkg.name)
     pkg.uuid !== nothing && push!(f, "uuid" => pkg.uuid)
     pkg.tree_hash !== nothing && push!(f, "tree_hash" => pkg.tree_hash)
-    pkg.path !== nothing && push!(f, "dev/path" => pkg.path)
+    pkg.path !== nothing && push!(f, "path" => pkg.path)
+    pkg.url !== nothing && push!(f, "url" => pkg.url)
+    pkg.rev !== nothing && push!(f, "rev" => pkg.rev)
+    pkg.subdir !== nothing && push!(f, "subdir" => pkg.subdir)
     pkg.pinned && push!(f, "pinned" => pkg.pinned)
     push!(f, "version" => (vstr == "VersionSpec(\"*\")" ? "*" : vstr))
     if pkg.repo.source !== nothing
-        push!(f, "url/path" => string("\"", pkg.repo.source, "\""))
+        push!(f, "repo/source" => string("\"", pkg.repo.source, "\""))
     end
     if pkg.repo.rev !== nothing
-        push!(f, "rev" => pkg.repo.rev)
+        push!(f, "repo/rev" => pkg.repo.rev)
     end
     if pkg.repo.subdir !== nothing
-        push!(f, "subdir" => pkg.repo.subdir)
+        push!(f, "repo/subdir" => pkg.repo.subdir)
     end
     print(io, "PackageSpec(\n")
     for (field, value) in f
@@ -830,7 +837,7 @@ end
 function ensure_resolved(manifest::Manifest,
         pkgs::AbstractVector{PackageSpec};
         registry::Bool=false,)::Nothing
-        unresolved_uuids = Dict{String,Vector{UUID}}()
+    unresolved_uuids = Dict{String,Vector{UUID}}()
     for pkg in pkgs
         has_uuid(pkg) && continue
         uuids = [uuid for (uuid, entry) in manifest if entry.name == pkg.name]

test/artifacts.jl

@@ -316,12 +316,12 @@ end
     # the global artifact namespace and package list, but it should be harmless.
     mktempdir() do project_path
         with_pkg_env(project_path) do
-            copy_test_package(project_path, "ArtifactInstallation")
+            path = git_init_package(project_path, joinpath(@__DIR__, "test_packages", "ArtifactInstallation"))
             add_this_pkg()
             Pkg.add(Pkg.Types.PackageSpec(
                 name="ArtifactInstallation",
                 uuid=Base.UUID("02111abe-2050-1119-117e-b30112b5bdc4"),
-                path=joinpath(project_path, "ArtifactInstallation"),
+                path=path,
             ))
 
             # Run test harness
@@ -635,6 +635,7 @@ end
         # loads overridden package artifacts.
         Pkg.activate(depot_container) do
             copy_test_package(depot_container, "ArtifactOverrideLoading")
+            git_init_and_commit(joinpath(depot_container, "ArtifactOverrideLoading"))
             add_this_pkg()
             Pkg.add(Pkg.Types.PackageSpec(
                 name="ArtifactOverrideLoading",

test/test_packages/ArtifactOverrideLoading/Project.toml

@@ -1,5 +1,5 @@
 name = "ArtifactOverrideLoading"
-uuid = "9f5278ee-142d-5d9a-9723-c4aebaa272e1"
+uuid = "7b879065-7f74-5fa4-bdd5-9b7a15df8941"
 version = "0.1.0"
 
 [deps]