From b9d1296950d82cc6585554a3aebb1a86b47f58de Mon Sep 17 00:00:00 2001 From: David Varela <00.varela.david@gmail.com> Date: Sat, 10 Apr 2021 10:50:31 -0700 Subject: [PATCH] Refactor: do not use double constructor for PackageSpec (#2477) * Refactor: do not use double constructor for PackageSpec * adjust artifact tests --- src/API.jl | 54 +++++++------------ src/Pkg.jl | 2 +- src/REPLMode/argument_parsers.jl | 24 +++++---- src/Types.jl | 19 ++++--- test/artifacts.jl | 5 +- .../ArtifactOverrideLoading/Project.toml | 2 +- 6 files changed, 51 insertions(+), 55 deletions(-) diff --git a/src/API.jl b/src/API.jl index 495f2635e9..15ba0036a4 100644 --- a/src/API.jl +++ b/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,19 +153,19 @@ 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 @@ -171,15 +173,13 @@ 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 diff --git a/src/Pkg.jl b/src/Pkg.jl index 919a02a244..2272626774 100644 --- a/src/Pkg.jl +++ b/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!(; diff --git a/src/REPLMode/argument_parsers.jl b/src/REPLMode/argument_parsers.jl index 4e66729487..0884937566 100644 --- a/src/REPLMode/argument_parsers.jl +++ b/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) diff --git a/src/Types.jl b/src/Types.jl index 67f7cc92e9..41b773e8e4 100644 --- a/src/Types.jl +++ b/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] diff --git a/test/artifacts.jl b/test/artifacts.jl index 7d72362754..3f4aa94954 100644 --- a/test/artifacts.jl +++ b/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", diff --git a/test/test_packages/ArtifactOverrideLoading/Project.toml b/test/test_packages/ArtifactOverrideLoading/Project.toml index 3a4f8757ab..2d567a8f2b 100644 --- a/test/test_packages/ArtifactOverrideLoading/Project.toml +++ b/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]