Avoid deleting existing artifacts when ignoring hashes. (#3768)

* avoid deleting existing artifacts

* fix order

* add testset

Author: nhz2

src/Artifacts.jl

@@ -49,12 +49,7 @@ function create_artifact(f::Function)
         # as something that was foolishly overridden.  This should be virtually impossible
         # unless the user has been very unwise, but let's be cautious.
         new_path = artifact_path(artifact_hash; honor_overrides=false)
-        if !isdir(new_path)
-            # Move this generated directory to its final destination, set it to read-only
-            mv(temp_dir, new_path)
-            chmod(new_path, filemode(dirname(new_path)))
-            set_readonly(new_path)
-        end
+        _mv_temp_artifact_dir(temp_dir, new_path)
 
         # Give the people what they want
         return artifact_hash
@@ -64,6 +59,28 @@ function create_artifact(f::Function)
     end
 end
 
+"""
+    _mv_temp_artifact_dir(temp_dir::String, new_path::String)::Nothing
+Either rename the directory at `temp_dir` to `new_path` and set it to read-only
+or if `new_path` artifact already exists try to do nothing.
+"""
+function _mv_temp_artifact_dir(temp_dir::String, new_path::String)::Nothing
+    if !isdir(new_path)
+        # This next step is like
+        # `mv(temp_dir, new_path)`.
+        # However, `mv` defaults to `cp` if `rename` returns an error.
+        # `cp` is not atomic, so avoid the potential of calling it.
+        err = ccall(:jl_fs_rename, Int32, (Cstring, Cstring), temp_dir, new_path)
+        # Ignore rename error, but ensure `new_path` exists.
+        if !isdir(new_path)
+            error("$(repr(new_path)) could not be made")
+        end
+        chmod(new_path, filemode(dirname(new_path)))
+        set_readonly(new_path)
+    end
+    nothing
+end
+
 """
     remove_artifact(hash::SHA1; honor_overrides::Bool=false)
 
@@ -289,68 +306,75 @@ function download_artifact(
         return true
     end
 
-    # We download by using `create_artifact()`.  We do this because the download may
+    # Ensure the `artifacts` directory exists in our default depot
+    artifacts_dir = first(artifacts_dirs())
+    mkpath(artifacts_dir)
+    # expected artifact path
+    dst = joinpath(artifacts_dir, bytes2hex(tree_hash.bytes))
+
+    # We download by using a temporary directory.  We do this because the download may
     # be corrupted or even malicious; we don't want to clobber someone else's artifact
     # by trusting the tree hash that has been given to us; we will instead download it
     # to a temporary directory, calculate the true tree hash, then move it to the proper
     # location only after knowing what it is, and if something goes wrong in the process,
-    # everything should be cleaned up.  Luckily, that is precisely what our
-    # `create_artifact()` wrapper does, so we use that here.
-    calc_hash = try
-        create_artifact() do dir
-            download_verify_unpack(tarball_url, tarball_hash, dir, ignore_existence=true, verbose=verbose,
+    # everything should be cleaned up.
+
+    # Temporary directory where we'll do our creation business
+    temp_dir = mktempdir(artifacts_dir)
+
+    try
+        download_verify_unpack(tarball_url, tarball_hash, temp_dir, ignore_existence=true, verbose=verbose,
                 quiet_download=quiet_download, io=io)
+        calc_hash = SHA1(GitTools.tree_hash(temp_dir))
+
+        # Did we get what we expected?  If not, freak out.
+        if calc_hash.bytes != tree_hash.bytes
+            msg = """
+            Tree Hash Mismatch!
+              Expected git-tree-sha1:   $(bytes2hex(tree_hash.bytes))
+              Calculated git-tree-sha1: $(bytes2hex(calc_hash.bytes))
+            """
+            # Since tree hash calculation is rather fragile and file system dependent,
+            # we allow setting JULIA_PKG_IGNORE_HASHES=1 to ignore the error and move
+            # the artifact to the expected location and return true
+            ignore_hash_env_set = get(ENV, "JULIA_PKG_IGNORE_HASHES", "") != ""
+            if ignore_hash_env_set
+                ignore_hash = Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", false)
+                ignore_hash === nothing && @error(
+                    "Invalid ENV[\"JULIA_PKG_IGNORE_HASHES\"] value",
+                    ENV["JULIA_PKG_IGNORE_HASHES"],
+                )
+                ignore_hash = something(ignore_hash, false)
+            else
+                # default: false except Windows users who can't symlink
+                ignore_hash = Sys.iswindows() &&
+                    !mktempdir(can_symlink, artifacts_dir)
+            end
+            if ignore_hash
+                desc = ignore_hash_env_set ?
+                    "Environment variable \$JULIA_PKG_IGNORE_HASHES is true" :
+                    "System is Windows and user cannot create symlinks"
+                msg *= "\n$desc: \
+                    ignoring hash mismatch and moving \
+                    artifact to the expected location"
+                @error(msg)
+            else
+                error(msg)
+            end
         end
+        # Move it to the location we expected
+        _mv_temp_artifact_dir(temp_dir, dst)
     catch err
         @debug "download_artifact error" tree_hash tarball_url tarball_hash err
         if isa(err, InterruptException)
             rethrow(err)
         end
         # If something went wrong during download, return the error
         return err
+    finally
+        # Always attempt to cleanup
+        rm(temp_dir; recursive=true, force=true)
     end
-
-    # Did we get what we expected?  If not, freak out.
-    if calc_hash.bytes != tree_hash.bytes
-        msg = """
-        Tree Hash Mismatch!
-          Expected git-tree-sha1:   $(bytes2hex(tree_hash.bytes))
-          Calculated git-tree-sha1: $(bytes2hex(calc_hash.bytes))
-        """
-        # actual and expected artifiact paths
-        src = artifact_path(calc_hash; honor_overrides=false)
-        dst = artifact_path(tree_hash; honor_overrides=false)
-        # Since tree hash calculation is rather fragile and file system dependent,
-        # we allow setting JULIA_PKG_IGNORE_HASHES=1 to ignore the error and move
-        # the artifact to the expected location and return true
-        ignore_hash_env_set = get(ENV, "JULIA_PKG_IGNORE_HASHES", "") != ""
-        if ignore_hash_env_set
-            ignore_hash = Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", false)
-            ignore_hash === nothing && @error(
-                "Invalid ENV[\"JULIA_PKG_IGNORE_HASHES\"] value",
-                ENV["JULIA_PKG_IGNORE_HASHES"],
-            )
-            ignore_hash = something(ignore_hash, false)
-        else
-            # default: false except Windows users who can't symlink
-            ignore_hash = Sys.iswindows() &&
-                !mktempdir(can_symlink, dirname(src))
-        end
-        if ignore_hash
-            desc = ignore_hash_env_set ?
-                "Environment variable \$JULIA_PKG_IGNORE_HASHES is true" :
-                "System is Windows and user cannot create symlinks"
-            msg *= "\n$desc: \
-                ignoring hash mismatch and moving \
-                artifact to the expected location"
-            @error(msg)
-            # Move it to the location we expected
-            mv(src, dst; force=true)
-            return true
-        end
-        return ErrorException(msg)
-    end
-
     return true
 end
 

test/artifacts.jl

@@ -799,4 +799,30 @@ end
     end
 end
 
+@testset "installing artifacts when symlinks are copied" begin
+    # copy symlinks to simulate the typical Microsoft Windows user experience where
+    # developer mode is not enabled (no admin rights)
+    withenv("BINARYPROVIDER_COPYDEREF"=>"true", "JULIA_PKG_IGNORE_HASHES"=>"true") do
+        temp_pkg_dir() do tmpdir
+            artifacts_toml = joinpath(tmpdir, "Artifacts.toml")
+            cp(joinpath(@__DIR__, "test_packages", "ArtifactInstallation", "Artifacts.toml"), artifacts_toml)
+            Pkg.activate(tmpdir)
+            cts_real_hash = create_artifact() do dir
+                local meta = Artifacts.artifact_meta("collapse_the_symlink", artifacts_toml)
+                local collapse_url = meta["download"][1]["url"]
+                local collapse_hash = meta["download"][1]["sha256"]
+                # Because "BINARYPROVIDER_COPYDEREF"=>"true", this will copy symlinks.
+                download_verify_unpack(collapse_url, collapse_hash, dir; verbose=true, ignore_existence=true)
+            end
+            cts_hash = artifact_hash("collapse_the_symlink", artifacts_toml)
+            @test !artifact_exists(cts_hash)
+            @test artifact_exists(cts_real_hash)
+            @test_logs (:error, r"Tree Hash Mismatch!") match_mode=:any Pkg.instantiate()
+            @test artifact_exists(cts_hash)
+            # Make sure existing artifacts don't get deleted.
+            @test artifact_exists(cts_real_hash)
+        end
+    end
+end
+
 end # module