make .path field consistently be relative manifest and convert to project relative upon writing to a project file (#4427)

Author: KristofferC

src/API.jl

@@ -225,7 +225,7 @@ function update_source_if_set(env, pkg)
         if pkg.subdir !== nothing
             source["subdir"] = pkg.subdir
         end
-        path, repo = get_path_repo(project, pkg.name)
+        path, repo = get_path_repo(project, env.project_file, env.manifest_file, pkg.name)
         if path !== nothing
             pkg.path = path
         end
@@ -397,13 +397,13 @@ end
 function append_all_pkgs!(pkgs, ctx, mode)
     if mode == PKGMODE_PROJECT || mode == PKGMODE_COMBINED
         for (name::String, uuid::UUID) in ctx.env.project.deps
-            path, repo = get_path_repo(ctx.env.project, name)
+            path, repo = get_path_repo(ctx.env.project, ctx.env.project_file, ctx.env.manifest_file, name)
             push!(pkgs, PackageSpec(name = name, uuid = uuid, path = path, repo = repo))
         end
     end
     if mode == PKGMODE_MANIFEST || mode == PKGMODE_COMBINED
         for (uuid, entry) in ctx.env.manifest
-            path, repo = get_path_repo(ctx.env.project, entry.name)
+            path, repo = get_path_repo(ctx.env.project, ctx.env.project_file, ctx.env.manifest_file, entry.name)
             push!(pkgs, PackageSpec(name = entry.name, uuid = uuid, path = path, repo = repo))
         end
     end

src/Operations.jl

@@ -148,7 +148,7 @@ function load_project_deps(
 
     for (name::String, uuid::UUID) in project.deps
         findfirst(pkg -> pkg.uuid == uuid, pkgs) === nothing || continue # do not duplicate packages
-        path, repo = get_path_repo(project, name)
+        path, repo = get_path_repo(project, project_file, manifest_file, name)
         entry = manifest_info(manifest, uuid)
         push!(
             pkgs_direct, entry === nothing ?
@@ -197,7 +197,7 @@ function load_all_deps(
     pkgs = load_manifest_deps(env.manifest, pkgs; preserve = preserve)
     # Sources takes presedence over the manifest...
     for pkg in pkgs
-        path, repo = get_path_repo(env.project, pkg.name)
+        path, repo = get_path_repo(env.project, env.project_file, env.manifest_file, pkg.name)
         if path !== nothing
             pkg.path = path
         end
@@ -363,7 +363,7 @@ function reset_all_compat!(proj::Project)
     return nothing
 end
 
-function collect_project(pkg::Union{PackageSpec, Nothing}, path::String, julia_version)
+function collect_project(pkg::Union{PackageSpec, Nothing}, path::String, manifest_file::String, julia_version)
     deps = PackageSpec[]
     weakdeps = Set{UUID}()
     project_file = projectfile_path(path; strict = true)
@@ -373,9 +373,9 @@ function collect_project(pkg::Union{PackageSpec, Nothing}, path::String, julia_v
         pkgerror("julia version requirement for package at `$path` not satisfied: compat entry \"julia = $(get_compat_str(project, "julia"))\" does not include Julia version $julia_version")
     end
     for (name, uuid) in project.deps
-        path, repo = get_path_repo(project, name)
+        dep_path, repo = get_path_repo(project, project_file, manifest_file, name)
         vspec = get_compat(project, name)
-        push!(deps, PackageSpec(name = name, uuid = uuid, version = vspec, path = path, repo = repo))
+        push!(deps, PackageSpec(name = name, uuid = uuid, version = vspec, path = dep_path, repo = repo))
     end
     for (name, uuid) in project.weakdeps
         vspec = get_compat(project, name)
@@ -439,15 +439,15 @@ function collect_fixed!(env::EnvCache, pkgs::Vector{PackageSpec}, names::Dict{UU
     weak_map = Dict{UUID, Set{UUID}}()
 
     uuid = Types.project_uuid(env)
-    deps, weakdeps = collect_project(env.pkg, dirname(env.project_file), julia_version)
+    deps, weakdeps = collect_project(env.pkg, dirname(env.project_file), env.manifest_file, julia_version)
     deps_map[uuid] = deps
     weak_map[uuid] = weakdeps
     names[uuid] = env.pkg === nothing ? "project" : env.pkg.name
 
     for (path, project) in env.workspace
         uuid = Types.project_uuid(project, path)
         pkg = project.name === nothing ? nothing : PackageSpec(name = project.name, uuid = uuid)
-        deps, weakdeps = collect_project(pkg, path, julia_version)
+        deps, weakdeps = collect_project(pkg, path, env.manifest_file, julia_version)
         deps_map[Types.project_uuid(env)] = deps
         weak_map[Types.project_uuid(env)] = weakdeps
         names[uuid] = project.name === nothing ? "project" : project.name
@@ -485,7 +485,7 @@ function collect_fixed!(env::EnvCache, pkgs::Vector{PackageSpec}, names::Dict{UU
             end
             pkgerror(error_msg)
         end
-        deps, weakdeps = collect_project(pkg, path, julia_version)
+        deps, weakdeps = collect_project(pkg, path, env.manifest_file, julia_version)
         deps_map[pkg.uuid] = deps
         weak_map[pkg.uuid] = weakdeps
     end
@@ -2081,7 +2081,7 @@ function up(
     # TODO check all pkg.version == VersionSpec()
     # set version constraints according to `level`
     for pkg in pkgs
-        source_path, source_repo = get_path_repo(ctx.env.project, pkg.name)
+        source_path, source_repo = get_path_repo(ctx.env.project, ctx.env.project_file, ctx.env.manifest_file, pkg.name)
         entry = manifest_info(ctx.env.manifest, pkg.uuid)
         new = up_load_versions!(ctx, pkg, entry, source_path, source_repo, level)
         new && push!(new_git, pkg.uuid) #TODO put download + push! in utility function
@@ -2333,7 +2333,8 @@ end
 function abspath!(env::EnvCache, project::Project)
     for (key, entry) in project.sources
         if haskey(entry, "path")
-            entry["path"] = project_rel_path(env, entry["path"])
+            # Paths in project sources are project-relative, so join with project_file dir, not manifest_file dir
+            entry["path"] = normpath(joinpath(dirname(env.project_file), entry["path"]))
         end
     end
     return project

src/Types.jl

@@ -442,6 +442,22 @@ function EnvCache(env::Union{Nothing, String} = nothing)
     return env′
 end
 
+# Convert a path from project-relative to manifest-relative
+# If path is absolute, returns it as-is
+function project_path_to_manifest_path(project_file::String, manifest_file::String, path::String)
+    isabspath(path) && return path
+    abs_path = Pkg.safe_realpath(joinpath(dirname(project_file), path))
+    return relpath(abs_path, Pkg.safe_realpath(dirname(manifest_file)))
+end
+
+# Convert a path from manifest-relative to project-relative
+# If path is absolute, returns it as-is
+function manifest_path_to_project_path(project_file::String, manifest_file::String, path::String)
+    isabspath(path) && return path
+    abs_path = Pkg.safe_realpath(joinpath(dirname(manifest_file), path))
+    return relpath(abs_path, Pkg.safe_realpath(dirname(project_file)))
+end
+
 include("project.jl")
 include("manifest.jl")
 
@@ -1281,7 +1297,7 @@ function write_env(
     )
     # Verify that the generated manifest is consistent with `sources`
     for (pkg, uuid) in env.project.deps
-        path, repo = get_path_repo(env.project, pkg)
+        path, repo = get_path_repo(env.project, env.project_file, env.manifest_file, pkg)
         entry = manifest_info(env.manifest, uuid)
         if path !== nothing
             @assert normpath(entry.path) == normpath(path)
@@ -1296,7 +1312,9 @@ function write_env(
         end
         if entry !== nothing
             if entry.path !== nothing
-                env.project.sources[pkg] = Dict("path" => entry.path)
+                # Convert path from manifest-relative to project-relative before writing
+                project_relative_path = manifest_path_to_project_path(env.project_file, env.manifest_file, entry.path)
+                env.project.sources[pkg] = Dict("path" => project_relative_path)
             elseif entry.repo != GitRepo()
                 d = Dict{String, String}()
                 entry.repo.source !== nothing && (d["url"] = entry.repo.source)

src/project.jl

@@ -4,7 +4,7 @@
 listed_deps(project::Project; include_weak::Bool) =
     vcat(collect(keys(project.deps)), collect(keys(project.extras)), include_weak ? collect(keys(project.weakdeps)) : String[])
 
-function get_path_repo(project::Project, name::String)
+function get_path_repo(project::Project, project_file::String, manifest_file::String, name::String)
     source = get(project.sources, name, nothing)
     if source === nothing
         return nothing, GitRepo()
@@ -17,6 +17,10 @@ function get_path_repo(project::Project, name::String)
         pkgerror("`path` and `url` are conflicting specifications")
     end
     repo = GitRepo(url, rev, subdir)
+    # Convert path from project-relative to manifest-relative
+    if path !== nothing
+        path = project_path_to_manifest_path(project_file, manifest_file, path)
+    end
     return path, repo
 end
 

test/workspaces.jl

@@ -26,8 +26,7 @@ temp_pkg_dir() do project_path
                 Pkg.develop(path = "PrivatePackage")
                 d = TOML.parsefile("Project.toml")
                 d["workspace"] = Dict("projects" => ["test", "docs", "benchmarks", "PrivatePackage"])
-                abs_path = abspath("PrivatePackage") # TODO: Make relative after #3842 is fixed
-                d["sources"] = Dict("PrivatePackage" => Dict("path" => abs_path))
+                d["sources"] = Dict("PrivatePackage" => Dict("path" => "PrivatePackage"))
                 Pkg.Types.write_project(d, "Project.toml")
                 write(
                     "src/MonorepoSub.jl", """
@@ -105,8 +104,7 @@ temp_pkg_dir() do project_path
                 Pkg.generate("TestSpecificPackage")
                 Pkg.develop(path = "TestSpecificPackage")
                 d = TOML.parsefile("test/Project.toml")
-                abs_pkg = abspath("TestSpecificPackage") # TODO: Make relative after #3842 is fixed
-                d["sources"] = Dict("TestSpecificPackage" => Dict("path" => abs_pkg))
+                d["sources"] = Dict("TestSpecificPackage" => Dict("path" => "../TestSpecificPackage"))
                 Pkg.Types.write_project(d, "test/Project.toml")
 
                 @test !isfile("test/Manifest.toml")