properly normalize paths before accessing them (#4463)

Author: KristofferC

src/API.jl

@@ -12,9 +12,9 @@ import FileWatching
 
 import Base: StaleCacheKey
 
-import ..depots, ..depots1, ..logdir, ..devdir, ..printpkgstyle, .._autoprecompilation_enabled_scoped
+import ..depots, ..depots1, ..logdir, ..devdir, ..printpkgstyle, .._autoprecompilation_enabled_scoped, ..manifest_rel_path
 import ..Operations, ..GitTools, ..Pkg, ..Registry
-import ..can_fancyprint, ..pathrepr, ..isurl, ..PREV_ENV_PATH, ..atomic_toml_write
+import ..can_fancyprint, ..pathrepr, ..isurl, ..PREV_ENV_PATH, ..atomic_toml_write, ..safe_realpath
 using ..Types, ..TOML
 using ..Types: VersionTypes
 using Base.BinaryPlatforms
@@ -64,7 +64,7 @@ end
 function package_info(env::EnvCache, pkg::PackageSpec, entry::PackageEntry)::PackageInfo
     git_source = pkg.repo.source === nothing ? nothing :
         isurl(pkg.repo.source::String) ? pkg.repo.source::String :
-        Operations.project_rel_path(env, pkg.repo.source::String)
+        safe_realpath(manifest_rel_path(env, pkg.repo.source::String))
     _source_path = Operations.source_path(env.manifest_file, pkg)
     if _source_path === nothing
         @debug "Manifest file $(env.manifest_file) contents:\n$(read(env.manifest_file, String))"
@@ -81,7 +81,7 @@ function package_info(env::EnvCache, pkg::PackageSpec, entry::PackageEntry)::Pac
         is_tracking_registry = Operations.is_tracking_registry(pkg),
         git_revision = pkg.repo.rev,
         git_source = git_source,
-        source = Operations.project_rel_path(env, _source_path),
+        source = _source_path,
         dependencies = copy(entry.deps), #TODO is copy needed?
     )
     return info
@@ -250,6 +250,19 @@ function update_source_if_set(env, pkg)
     return
 end
 
+# Normalize relative paths from user input (pwd-relative) to internal representation (manifest-relative)
+# This ensures all relative paths in Pkg are consistently relative to the manifest file
+function normalize_package_paths!(ctx::Context, pkgs::Vector{PackageSpec})
+    for pkg in pkgs
+        if pkg.repo.source !== nothing && !isurl(pkg.repo.source) && !isabspath(pkg.repo.source)
+            # User provided a relative path (relative to pwd), convert to manifest-relative
+            absolute_path = abspath(pkg.repo.source)
+            pkg.repo.source = Types.relative_project_path(ctx.env.manifest_file, absolute_path)
+        end
+    end
+    return
+end
+
 function develop(
         ctx::Context, pkgs::Vector{PackageSpec}; shared::Bool = true,
         preserve::PreserveLevel = Operations.default_preserve(), platform::AbstractPlatform = HostPlatform(), kwargs...
@@ -285,6 +298,8 @@ function develop(
         end
     end
 
+    normalize_package_paths!(ctx, pkgs)
+
     new_git = handle_repos_develop!(ctx, pkgs, shared)
 
     Operations.update_registries(ctx; force = false, update_cooldown = Day(1))
@@ -337,6 +352,8 @@ function add(
         end
     end
 
+    normalize_package_paths!(ctx, pkgs)
+
     repo_pkgs = PackageSpec[pkg for pkg in pkgs if (pkg.repo.source !== nothing || pkg.repo.rev !== nothing)]
     new_git = handle_repos_add!(ctx, repo_pkgs)
     # repo + unpinned -> name, uuid, repo.rev, repo.source, tree_hash
@@ -782,7 +799,15 @@ function gc(ctx::Context = Context(); collect_delay::Union{Period, Nothing} = no
         end
 
         # Collect the locations of every repo referred to in this manifest
-        return [Types.add_repo_cache_path(e.repo.source) for (u, e) in manifest if e.repo.source !== nothing]
+        return [
+            Types.add_repo_cache_path(
+                    isurl(e.repo.source) ? e.repo.source :
+                    safe_realpath(
+                        isabspath(e.repo.source) ? e.repo.source :
+                        normpath(joinpath(dirname(path), e.repo.source))
+                    )
+                ) for (u, e) in manifest if e.repo.source !== nothing
+        ]
     end
 
     function process_artifacts_toml(path, pkgs_to_delete)
@@ -1314,7 +1339,7 @@ function instantiate(
         ## Download repo at tree hash
         # determine canonical form of repo source
         if !isurl(repo_source)
-            repo_source = normpath(joinpath(dirname(ctx.env.project_file), repo_source))
+            repo_source = manifest_rel_path(ctx.env, repo_source)
         end
         if !isurl(repo_source) && !isdir(repo_source)
             pkgerror("Did not find path `$(repo_source)` for $(err_rep(pkg))")

src/Operations.jl

@@ -16,7 +16,7 @@ using Base.BinaryPlatforms
 import ...Pkg
 import ...Pkg: pkg_server, Registry, pathrepr, can_fancyprint, printpkgstyle, stderr_f, OFFLINE_MODE
 import ...Pkg: UPDATED_REGISTRY_THIS_SESSION, RESPECT_SYSIMAGE_VERSIONS, should_autoprecompile
-import ...Pkg: usable_io, discover_repo, create_cachedir_tag
+import ...Pkg: usable_io, discover_repo, create_cachedir_tag, manifest_rel_path
 
 #########
 # Utils #
@@ -130,7 +130,7 @@ end
 
 function source_path(manifest_file::String, pkg::Union{PackageSpec, PackageEntry}, julia_version = VERSION)
     return pkg.tree_hash !== nothing ? find_installed(pkg.name, pkg.uuid, pkg.tree_hash) :
-        pkg.path !== nothing ? joinpath(dirname(manifest_file), pkg.path) :
+        pkg.path !== nothing ? normpath(joinpath(dirname(manifest_file), pkg.path)) :
         is_or_was_stdlib(pkg.uuid, julia_version) ? Types.stdlib_path(pkg.name) :
         nothing
 end
@@ -540,7 +540,7 @@ is_tracking_registry(pkg) = !is_tracking_path(pkg) && !is_tracking_repo(pkg)
 isfixed(pkg) = !is_tracking_registry(pkg) || pkg.pinned
 
 function collect_developed!(env::EnvCache, pkg::PackageSpec, developed::Vector{PackageSpec})
-    source = project_rel_path(env, source_path(env.manifest_file, pkg))
+    source = source_path(env.manifest_file, pkg)
     source_env = EnvCache(projectfile_path(source))
     pkgs = load_project_deps(source_env.project, source_env.project_file, source_env.manifest, source_env.manifest_file)
     for pkg in pkgs
@@ -553,10 +553,7 @@ function collect_developed!(env::EnvCache, pkg::PackageSpec, developed::Vector{P
             # otherwise relative to manifest file....
             pkg.path = Types.relative_project_path(
                 env.manifest_file,
-                project_rel_path(
-                    source_env,
-                    source_path(source_env.manifest_file, pkg)
-                )
+                source_path(source_env.manifest_file, pkg)
             )
             push!(developed, pkg)
             collect_developed!(env, pkg, developed)
@@ -607,13 +604,13 @@ function collect_fixed!(env::EnvCache, pkgs::Vector{PackageSpec}, names::Dict{UU
         pkg.uuid === nothing && continue
         # add repo package if necessary
         source = source_path(env.manifest_file, pkg)
-        path = source === nothing ? nothing : project_rel_path(env, source)
+        path = source
         if (path === nothing || !isdir(path)) && (pkg.repo.rev !== nothing || pkg.repo.source !== nothing)
             # ensure revved package is installed
             # pkg.tree_hash is set in here
             Types.handle_repo_add!(Types.Context(env = env), pkg)
             # Recompute path
-            path = project_rel_path(env, source_path(env.manifest_file, pkg))
+            path = source_path(env.manifest_file, pkg)
         end
         if !isdir(path)
             # Find which packages depend on this missing package for better error reporting
@@ -1538,7 +1535,6 @@ end
 ################################
 # Manifest update and pruning #
 ################################
-project_rel_path(env::EnvCache, path::String) = normpath(joinpath(dirname(env.manifest_file), path))
 
 function prune_manifest(env::EnvCache)
     # if project uses another manifest, only prune project entry in manifest
@@ -2589,7 +2585,7 @@ end
 function abspath!(env::EnvCache, manifest::Manifest)
     for (uuid, entry) in manifest
         if entry.path !== nothing
-            entry.path = project_rel_path(env, entry.path)
+            entry.path = manifest_rel_path(env, entry.path)
         end
     end
     return manifest
@@ -2816,7 +2812,7 @@ function test(
     missing_runtests = String[]
     source_paths = String[] # source_path is the package root (not /src)
     for pkg in pkgs
-        sourcepath = project_rel_path(ctx.env, source_path(ctx.env.manifest_file, pkg, ctx.julia_version)) # TODO
+        sourcepath = source_path(ctx.env.manifest_file, pkg, ctx.julia_version)
         !isfile(testfile(sourcepath)) && push!(missing_runtests, pkg.name)
         push!(source_paths, sourcepath)
     end

src/Types.jl

@@ -801,8 +801,17 @@ end
 function handle_repo_develop!(ctx::Context, pkg::PackageSpec, shared::Bool)
     # First, check if we can compute the path easily (which requires a given local path or name)
     is_local_path = pkg.repo.source !== nothing && !isurl(pkg.repo.source)
+    # Preserve whether the original source was an absolute path - needed later to decide how to store the path
+    original_source_was_absolute = is_local_path && isabspath(pkg.repo.source)
+
     if is_local_path || pkg.name !== nothing
-        dev_path = is_local_path ? pkg.repo.source : devpath(ctx.env, pkg.name, shared)
+        # Resolve manifest-relative paths to absolute paths for file system operations
+        dev_path = if is_local_path
+            isabspath(pkg.repo.source) ? pkg.repo.source :
+                Pkg.manifest_rel_path(ctx.env, pkg.repo.source)
+        else
+            devpath(ctx.env, pkg.name, shared)
+        end
         if pkg.repo.subdir !== nothing
             dev_path = joinpath(dev_path, pkg.repo.subdir)
         end
@@ -818,7 +827,7 @@ function handle_repo_develop!(ctx::Context, pkg::PackageSpec, shared::Bool)
             resolve_projectfile!(pkg, dev_path)
             error_if_in_sysimage(pkg)
             if is_local_path
-                pkg.path = isabspath(dev_path) ? dev_path : relative_project_path(ctx.env.manifest_file, dev_path)
+                pkg.path = original_source_was_absolute ? dev_path : relative_project_path(ctx.env.manifest_file, dev_path)
             else
                 pkg.path = shared ? dev_path : relative_project_path(ctx.env.manifest_file, dev_path)
             end
@@ -847,7 +856,11 @@ function handle_repo_develop!(ctx::Context, pkg::PackageSpec, shared::Bool)
     cloned = false
     package_path = pkg.repo.subdir === nothing ? repo_path : joinpath(repo_path, pkg.repo.subdir)
     if !has_name(pkg)
-        LibGit2.close(GitTools.ensure_clone(ctx.io, repo_path, pkg.repo.source))
+        # Resolve manifest-relative path to absolute before passing to git
+        repo_source_resolved = !isurl(pkg.repo.source) && !isabspath(pkg.repo.source) ?
+            Pkg.manifest_rel_path(ctx.env, pkg.repo.source) :
+            pkg.repo.source
+        LibGit2.close(GitTools.ensure_clone(ctx.io, repo_path, repo_source_resolved))
         cloned = true
         resolve_projectfile!(pkg, package_path)
     end
@@ -870,7 +883,11 @@ function handle_repo_develop!(ctx::Context, pkg::PackageSpec, shared::Bool)
     else
         mkpath(dirname(dev_path))
         if !cloned
-            LibGit2.close(GitTools.ensure_clone(ctx.io, dev_path, pkg.repo.source))
+            # Resolve manifest-relative path to absolute before passing to git
+            repo_source_resolved = !isurl(pkg.repo.source) && !isabspath(pkg.repo.source) ?
+                Pkg.manifest_rel_path(ctx.env, pkg.repo.source) :
+                pkg.repo.source
+            LibGit2.close(GitTools.ensure_clone(ctx.io, dev_path, repo_source_resolved))
         else
             mv(repo_path, dev_path)
         end
@@ -880,7 +897,13 @@ function handle_repo_develop!(ctx::Context, pkg::PackageSpec, shared::Bool)
         resolve_projectfile!(pkg, joinpath(dev_path, pkg.repo.subdir === nothing ? "" : pkg.repo.subdir))
     end
     error_if_in_sysimage(pkg)
-    pkg.path = shared ? dev_path : relative_project_path(ctx.env.manifest_file, dev_path)
+    # When an explicit local path was given, preserve whether it was absolute or relative
+    # Otherwise, use shared flag to determine if path should be absolute (shared) or relative (local)
+    if is_local_path
+        pkg.path = original_source_was_absolute ? dev_path : relative_project_path(ctx.env.manifest_file, dev_path)
+    else
+        pkg.path = shared ? dev_path : relative_project_path(ctx.env.manifest_file, dev_path)
+    end
     if pkg.repo.subdir !== nothing
         pkg.path = joinpath(pkg.path, pkg.repo.subdir)
     end
@@ -948,10 +971,12 @@ function handle_repo_add!(ctx::Context, pkg::PackageSpec)
     @assert pkg.repo.source !== nothing
 
     # We now have the source of the package repo, check if it is a local path and if that exists
-    repo_source = pkg.repo.source
+    repo_source = !isurl(pkg.repo.source) && !isabspath(pkg.repo.source) ?
+        normpath(joinpath(dirname(ctx.env.manifest_file), pkg.repo.source)) :
+        pkg.repo.source
     if !isurl(pkg.repo.source)
-        if isdir(pkg.repo.source)
-            git_path = joinpath(pkg.repo.source, ".git")
+        if isdir(repo_source)
+            git_path = joinpath(repo_source, ".git")
             if isfile(git_path)
                 # Git submodule: .git is a file containing path to actual git directory
                 git_ref_content = readline(git_path)
@@ -961,22 +986,25 @@ function handle_repo_add!(ctx::Context, pkg::PackageSpec)
                 git_info_path = git_path
             end
             if !isdir(git_info_path)
-                msg = "Did not find a git repository at `$(pkg.repo.source)`"
-                if isfile(joinpath(pkg.repo.source, "Project.toml")) || isfile(joinpath(pkg.repo.source, "JuliaProject.toml"))
+                msg = "Did not find a git repository at `$(repo_source)`"
+                if isfile(joinpath(repo_source, "Project.toml")) || isfile(joinpath(repo_source, "JuliaProject.toml"))
                     msg *= ", perhaps you meant `Pkg.develop`?"
                 end
                 pkgerror(msg)
             end
-            LibGit2.with(GitTools.check_valid_HEAD, LibGit2.GitRepo(pkg.repo.source)) # check for valid git HEAD
-            LibGit2.with(LibGit2.GitRepo(pkg.repo.source)) do repo
+            LibGit2.with(GitTools.check_valid_HEAD, LibGit2.GitRepo(repo_source)) # check for valid git HEAD
+            LibGit2.with(LibGit2.GitRepo(repo_source)) do repo
                 if LibGit2.isdirty(repo)
-                    @warn "The repository at `$(pkg.repo.source)` has uncommitted changes. Consider using `Pkg.develop` instead of `Pkg.add` if you want to work with the current state of the repository."
+                    @warn "The repository at `$(repo_source)` has uncommitted changes. Consider using `Pkg.develop` instead of `Pkg.add` if you want to work with the current state of the repository."
                 end
             end
-            pkg.repo.source = isabspath(pkg.repo.source) ? safe_realpath(pkg.repo.source) : relative_project_path(ctx.env.manifest_file, pkg.repo.source)
-            repo_source = normpath(joinpath(dirname(ctx.env.manifest_file), pkg.repo.source))
+            # Store the path: use the original path format (absolute vs relative) as the user provided
+            # Canonicalize repo_source for consistent hashing in cache paths
+            repo_source = safe_realpath(repo_source)
+            pkg.repo.source = isabspath(pkg.repo.source) ? repo_source : relative_project_path(ctx.env.manifest_file, repo_source)
         else
-            pkgerror("Path `$(pkg.repo.source)` does not exist.")
+            # For error messages, show the absolute path which is more informative than manifest-relative
+            pkgerror("Path `$(repo_source)` does not exist.")
         end
     end
 

src/utils.jl

@@ -212,3 +212,7 @@ function discover_repo(path::AbstractString)
     end
     return
 end
+
+# Resolve a manifest-relative path to an absolute path
+# Note: Despite the name "manifest_rel_path", this resolves relative to the manifest file
+manifest_rel_path(env, path::String) = normpath(joinpath(dirname(env.manifest_file), path))

test/new.jl

@@ -1496,7 +1496,7 @@ end
         cd_tempdir() do dir
             # adding a nonexistent directory
             @test_throws PkgError(
-                "Path `$(normpath("some/really/random/Dir"))` does not exist."
+                "Path `$(abspath("some/really/random/Dir"))` does not exist."
             ) Pkg.pkg"add some/really/random/Dir"
             # warn if not explicit about adding directory
             mkdir("Example")

test/pkg.jl

@@ -1182,4 +1182,56 @@ end
     end
 end
 
+# issue #2291: relative paths in manifests should be resolved relative to manifest location
+@testset "relative path resolution from different directories (issue #2291)" begin
+    mktempdir() do dir
+        # Create a local package with a git repo
+        pkg_path = joinpath(dir, "LocalPackage")
+        mkpath(joinpath(pkg_path, "src"))
+        write(
+            joinpath(pkg_path, "Project.toml"), """
+            name = "LocalPackage"
+            uuid = "00000000-0000-0000-0000-000000000001"
+            version = "0.1.0"
+            """
+        )
+        write(
+            joinpath(pkg_path, "src", "LocalPackage.jl"), """
+            module LocalPackage
+            greet() = "Hello from LocalPackage!"
+            end
+            """
+        )
+
+        # Initialize git repo
+        LibGit2.with(LibGit2.init(pkg_path)) do repo
+            LibGit2.add!(repo, "*")
+            LibGit2.commit(repo, "Initial commit"; author = TEST_SIG, committer = TEST_SIG)
+        end
+
+        # Create a project in a subdirectory and add the package with relative path
+        project_path = joinpath(dir, "project")
+        mkpath(project_path)
+        cd(project_path) do
+            Pkg.activate(".")
+            Pkg.add(Pkg.PackageSpec(path = "../LocalPackage"))
+
+            # Verify the package was added with relative path
+            manifest = read_manifest(joinpath(project_path, "Manifest.toml"))
+            pkg_entry = manifest[UUID("00000000-0000-0000-0000-000000000001")]
+            @test pkg_entry.repo.source == "../LocalPackage"
+        end
+
+        # Now change to parent directory and try to update - this should work
+        cd(dir) do
+            Pkg.activate("project")
+            Pkg.update()  # This should not fail
+            # Check the package is installed by looking it up in dependencies
+            pkg_info = Pkg.dependencies()[UUID("00000000-0000-0000-0000-000000000001")]
+            @test pkg_info.name == "LocalPackage"
+            @test isinstalled(PackageSpec(uuid = UUID("00000000-0000-0000-0000-000000000001"), name = "LocalPackage"))
+        end
+    end
+end
+
 end # module