rebase 2135 (#2454)

get rid of the mode field in PackageSpec Co-authored-by: Kristoffer Carlsson <[email protected]> Co-authored-by: David Varela <[email protected]>
JuliaLang · Mar 31, 2021 · 9cf608c · 9cf608c
1 parent 3c9188f
commit 9cf608c
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,3 +6,4 @@ Pkg v1.7 Release Notes
 - Registries downloaded from Pkg Server (not git) is now assumed to be immutable. Manual changes to their files might not be picked up by a running Pkg session.
 - Adding packages by folder name in the REPL mode now requires a prepending a `./` to the folder name package folder is in the current folder, e.g. `add ./Package` is required instead of `add Pacakge`. This is to avoid confusion between the package name `Package` and the local directory `Package`.
 - `rm`, `pin`, and `free` now support the `--all` option, and the api variants gain the `all_pkgs::Bool` kwarg, to perform the operation on all packages within the project or manifest, depending on the mode of the operation.
+- The `mode` keyword for `PackageSpec` has been removed.
diff --git a/src/API.jl b/src/API.jl
@@ -151,10 +151,8 @@ 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, mode=mode, subdir=subdir)
-            # Pkg.status takes a mode argument as well which is a bit ambiguous with the
-            # mode argument to the PackageSpec but probably not a problem in practice
-            if $f === status
+            pkg = Package(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
@@ -278,7 +276,6 @@ function rm(ctx::Context, pkgs::Vector{PackageSpec}; mode=PKGMODE_PROJECT, all_p
     end
     require_not_empty(pkgs, :rm)
     pkgs = deepcopy(pkgs)  # deepcopy for avoid mutating PackageSpec members
-    foreach(pkg -> pkg.mode = mode, pkgs)
 
     for pkg in pkgs
         if pkg.name === nothing && pkg.uuid === nothing
@@ -293,11 +290,11 @@ function rm(ctx::Context, pkgs::Vector{PackageSpec}; mode=PKGMODE_PROJECT, all_p
 
     Context!(ctx; kwargs...)
 
-    project_deps_resolve!(ctx.env, pkgs)
-    manifest_resolve!(ctx.env.manifest, pkgs)
+    mode == PKGMODE_PROJECT && project_deps_resolve!(ctx.env, pkgs)
+    mode == PKGMODE_MANIFEST && manifest_resolve!(ctx.env.manifest, pkgs)
     ensure_resolved(ctx.env.manifest, pkgs)
 
-    Operations.rm(ctx, pkgs)
+    Operations.rm(ctx, pkgs; mode)
     return
 end
 
@@ -319,7 +316,6 @@ 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
-    foreach(pkg -> pkg.mode = mode, pkgs)
 
     Context!(ctx; kwargs...)
     if update_registry
@@ -331,6 +327,8 @@ function up(ctx::Context, pkgs::Vector{PackageSpec};
     if isempty(pkgs)
         append_all_pkgs!(pkgs, ctx, mode)
     else
+        mode == PKGMODE_PROJECT && project_deps_resolve!(ctx.env, pkgs)
+        mode == PKGMODE_MANIFEST && manifest_resolve!(ctx.env.manifest, pkgs)
         project_deps_resolve!(ctx.env, pkgs)
         manifest_resolve!(ctx.env.manifest, pkgs)
         ensure_resolved(ctx.env.manifest, pkgs)
@@ -371,7 +369,6 @@ function pin(ctx::Context, pkgs::Vector{PackageSpec}; all_pkgs::Bool=false, kwar
         end
     end
 
-    foreach(pkg -> pkg.mode = PKGMODE_PROJECT, pkgs)
     project_deps_resolve!(ctx.env, pkgs)
     ensure_resolved(ctx.env.manifest, pkgs)
     Operations.pin(ctx, pkgs)
@@ -398,7 +395,6 @@ function free(ctx::Context, pkgs::Vector{PackageSpec}; all_pkgs::Bool=false, kwa
         end
     end
 
-    foreach(pkg -> pkg.mode = PKGMODE_MANIFEST, pkgs)
     manifest_resolve!(ctx.env.manifest, pkgs)
     ensure_resolved(ctx.env.manifest, pkgs)
 
@@ -989,7 +985,6 @@ function build(ctx::Context, pkgs::Vector{PackageSpec}; verbose=false, kwargs...
         end
     end
     project_resolve!(ctx.env, pkgs)
-    foreach(pkg -> pkg.mode = PKGMODE_MANIFEST, pkgs)
     manifest_resolve!(ctx.env.manifest, pkgs)
     ensure_resolved(ctx.env.manifest, pkgs)
     Operations.build(ctx, Set{UUID}(pkg.uuid for pkg in pkgs), verbose)
@@ -1649,15 +1644,15 @@ end
 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, mode::PackageMode = PKGMODE_PROJECT,
+                 url = nothing, rev = nothing, path=nothing,
                  subdir = nothing)
     if path !== nothing && 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, mode=mode, path=nothing,
+    PackageSpec(;name=name, uuid=uuid, version=version, path=nothing,
                 repo=repo, tree_hash=nothing)
 end
 Package(name::AbstractString) = PackageSpec(name)

diff --git a/src/Operations.jl b/src/Operations.jl
@@ -955,17 +955,18 @@ end
 ##############
 # Operations #
 ##############
-function rm(ctx::Context, pkgs::Vector{PackageSpec})
+function rm(ctx::Context, pkgs::Vector{PackageSpec}; mode::PackageMode)
     drop = UUID[]
     # find manifest-mode drops
-    for pkg in pkgs
-        pkg.mode == PKGMODE_MANIFEST || continue
-        info = manifest_info(ctx.env.manifest, pkg.uuid)
-        if info !== nothing
-            pkg.uuid in drop || push!(drop, pkg.uuid)
-        else
-            str = has_name(pkg) ? pkg.name : string(pkg.uuid)
-            @warn("`$str` not in manifest, ignoring")
+    if mode == PKGMODE_MANIFEST
+        for pkg in pkgs
+            info = manifest_info(ctx.env.manifest, pkg.uuid)
+            if info !== nothing
+                pkg.uuid in drop || push!(drop, pkg.uuid)
+            else
+                str = has_name(pkg) ? pkg.name : string(pkg.uuid)
+                @warn("`$str` not in manifest, ignoring")
+            end
         end
     end
     # drop reverse dependencies
@@ -981,22 +982,23 @@ function rm(ctx::Context, pkgs::Vector{PackageSpec})
         clean && break
     end
     # find project-mode drops
-    for pkg in pkgs
-        pkg.mode == PKGMODE_PROJECT || continue
-        found = false
-        for (name::String, uuid::UUID) in ctx.env.project.deps
-            pkg.name == name || pkg.uuid == uuid || continue
-            pkg.name == name ||
-                error("project file name mismatch for `$uuid`: $(pkg.name) ≠ $name")
-            pkg.uuid == uuid ||
-                error("project file UUID mismatch for `$name`: $(pkg.uuid) ≠ $uuid")
-            uuid in drop || push!(drop, uuid)
-            found = true
-            break
+    if mode == PKGMODE_PROJECT
+        for pkg in pkgs
+            found = false
+            for (name::String, uuid::UUID) in ctx.env.project.deps
+                pkg.name == name || pkg.uuid == uuid || continue
+                pkg.name == name ||
+                    error("project file name mismatch for `$uuid`: $(pkg.name) ≠ $name")
+                pkg.uuid == uuid ||
+                    error("project file UUID mismatch for `$name`: $(pkg.uuid) ≠ $uuid")
+                uuid in drop || push!(drop, uuid)
+                found = true
+                break
+            end
+            found && continue
+            str = has_name(pkg) ? pkg.name : string(pkg.uuid)
+            @warn("`$str` not in project, ignoring")
         end
-        found && continue
-        str = has_name(pkg) ? pkg.name : string(pkg.uuid)
-        @warn("`$str` not in project, ignoring")
     end
     # delete drops from project
     n = length(ctx.env.project.deps)

diff --git a/src/Pkg.jl b/src/Pkg.jl
@@ -70,7 +70,7 @@ An enum with the instances
   * `PKGMODE_PROJECT`
 
 Determines if operations should be made on a project or manifest level.
-Used as an argument to  [`PackageSpec`](@ref) or as an argument to [`Pkg.rm`](@ref).
+Used as an argument to [`Pkg.rm`](@ref), [`Pkg.update`](@ref) and [`Pkg.status`](@ref).
 """
 const PackageMode = Types.PackageMode
 
@@ -155,10 +155,10 @@ Pkg.precompile()
 const precompile = API.precompile
 
 """
-    Pkg.rm(pkg::Union{String, Vector{String}})
-    Pkg.rm(pkg::Union{PackageSpec, Vector{PackageSpec}})
+    Pkg.rm(pkg::Union{String, Vector{String}}; mode::PackageMode = PKGMODE_PROJECT)
+    Pkg.rm(pkg::Union{PackageSpec, Vector{PackageSpec}}; mode::PackageMode = PKGMODE_PROJECT)
 
-Remove a package from the current project. If the `mode` of `pkg` is
+Remove a package from the current project. If `mode` is equal to
 `PKGMODE_MANIFEST` also remove it from the manifest including all
 recursive dependencies of `pkg`.
 
@@ -469,8 +469,6 @@ This includes:
   * A `url` and an optional git `rev`ision. `rev` can be a branch name or a git commit SHA1.
   * A local `path`. This is equivalent to using the `url` argument but can be more descriptive.
   * A `subdir` which can be used when adding a package that is not in the root of a repository.
-  * A `mode`, which is an instance of the enum [`PackageMode`](@ref), with possible values `PKGMODE_PROJECT`
-   (the default) or `PKGMODE_MANIFEST`. Used in e.g. [`Pkg.rm`](@ref).
 
 Most functions in Pkg take a `Vector` of `PackageSpec` and do the operation on all the packages
 in the vector.
@@ -495,7 +493,6 @@ Below is a comparison between the REPL mode and the functional API:
 | `Package#master`     | `PackageSpec(name="Package", rev="master")`           |
 | `local/path#feature` | `PackageSpec(path="local/path"; rev="feature")`       |
 | `www.mypkg.com`      | `PackageSpec(url="www.mypkg.com")`                    |
-| `--manifest Package` | `PackageSpec(name="Package", mode=PKGSPEC_MANIFEST)`  |
 | `--major Package`    | `PackageSpec(name="Package", version=PKGLEVEL_MAJOR)` |
 
 """

diff --git a/src/Types.jl b/src/Types.jl
@@ -96,7 +96,6 @@ Base.@kwdef mutable struct PackageSpec
     repo::GitRepo = GitRepo()
     path::Union{Nothing,String} = nothing
     pinned::Bool = false
-    mode::PackageMode = PKGMODE_PROJECT
 end
 PackageSpec(name::AbstractString) = PackageSpec(;name=name)
 PackageSpec(name::AbstractString, uuid::UUID) = PackageSpec(;name=name, uuid=uuid)
@@ -106,7 +105,7 @@ PackageSpec(n::AbstractString, u::UUID, v::VersionTypes) = PackageSpec(;name=n,
 function Base.:(==)(a::PackageSpec, b::PackageSpec)
     return a.name == b.name && a.uuid == b.uuid && a.version == b.version &&
     a.tree_hash == b.tree_hash && a.repo == b.repo && a.path == b.path &&
-    a.pinned == b.pinned && a.mode == b.mode
+    a.pinned == b.pinned
 end
 
 function err_rep(pkg::PackageSpec)
@@ -756,7 +755,6 @@ function project_deps_resolve!(env::EnvCache, pkgs::AbstractVector{PackageSpec})
     uuids = env.project.deps
     names = Dict(uuid => name for (name, uuid) in uuids)
     for pkg in pkgs
-        pkg.mode == PKGMODE_PROJECT || continue
         if has_name(pkg) && !has_uuid(pkg) && pkg.name in keys(uuids)
             pkg.uuid = uuids[pkg.name]
         end
@@ -775,7 +773,6 @@ function manifest_resolve!(manifest::Manifest, pkgs::AbstractVector{PackageSpec}
         names[uuid] = entry.name # can be duplicate but doesn't matter
     end
     for pkg in pkgs
-        force || pkg.mode == PKGMODE_MANIFEST || continue
         if has_name(pkg) && !has_uuid(pkg) && pkg.name in keys(uuids)
             length(uuids[pkg.name]) == 1 && (pkg.uuid = uuids[pkg.name][1])
         end

diff --git a/test/new.jl b/test/new.jl
@@ -1765,6 +1765,20 @@ end
         @test !haskey(Pkg.dependencies(), exuuid)
         @test haskey(Pkg.dependencies(), json_uuid)
     end end
+    # rm manifest mode
+    isolate(loaded_depot=true) do
+        Pkg.add("Example")
+        Pkg.add(name="JSON", version="0.18.0")
+        Pkg.rm("Random"; mode=Pkg.PKGMODE_MANIFEST)
+        @test haskey(Pkg.dependencies(), exuuid)
+        @test !haskey(Pkg.dependencies(), json_uuid)
+    end
+    # rm nonexistent packages warns but does not error
+    isolate(loaded_depot=true) do
+        Pkg.add("Example")
+        @test_logs (:warn, r"not in project, ignoring") Pkg.rm(name="FooBar", uuid=UUIDs.UUID(0))
+        @test_logs (:warn, r"not in manifest, ignoring") Pkg.rm(name="FooBar", uuid=UUIDs.UUID(0); mode=Pkg.PKGMODE_MANIFEST)
+    end
 end
 
 @testset "rm: REPL" begin