Don't clear executable bits in set_readonly() on Windows (#3349)

* Don't clear executable bits in `set_readonly()` on Windows

On windows, `filemode()` doesn't include executable bits.  This is
already worked around in `gitmode()` but we don't quite want to do
something as intrusive as `gitmode()` in `set_readonly()`; let's just
work around this one platform-specific foible.

This fixes problems like executable permissions disappearing when
creating artifacts on Windows, packages that contain executable files
losing those permissions on install, etc...

* Blindly try running all tests on windows now

* Get rid of special-case on Windows

* Update Artifacts.jl

* Update Artifacts.jl

Author: staticfloat

src/Artifacts.jl

@@ -287,71 +287,46 @@ function download_artifact(
         return true
     end
 
-    if Sys.iswindows()
-        # The destination directory we're hoping to fill:
-        dest_dir = artifact_path(tree_hash; honor_overrides=false)
-        mkpath(dest_dir)
-
-        # On Windows, we have some issues around stat() and chmod() that make properly
-        # determining the git tree hash problematic; for this reason, we use the "unsafe"
-        # artifact unpacking method, which does not properly verify unpacked git tree
-        # hash.  This will be fixed in a future Julia release which will properly interrogate
-        # the filesystem ACLs for executable permissions, which git tree hashes care about.
-        try
-            download_verify_unpack(tarball_url, tarball_hash, dest_dir, ignore_existence=true,
-                                   verbose=verbose, quiet_download=quiet_download, io=io)
-        catch err
-            @debug "download_artifact error" tree_hash tarball_url tarball_hash err
-            # Clean that destination directory out if something went wrong
-            rm(dest_dir; force=true, recursive=true)
-
-            if isa(err, InterruptException)
-                rethrow(err)
-            end
-            return err
+    # We download by using `create_artifact()`.  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,
+                quiet_download=quiet_download, io=io)
         end
-    else
-        # We download by using `create_artifact()`.  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,
-                    quiet_download=quiet_download, io=io)
-            end
-        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
+    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
+    end
 
-        # Did we get what we expected?  If not, freak out.
-        if calc_hash.bytes != tree_hash.bytes
-            msg  = "Tree Hash Mismatch!\n"
-            msg *= "  Expected git-tree-sha1:   $(bytes2hex(tree_hash.bytes))\n"
-            msg *= "  Calculated git-tree-sha1: $(bytes2hex(calc_hash.bytes))"
-            # Since tree hash calculation is still broken on some systems, e.g. Pkg.jl#1860,
-            # and Pkg.jl#2317 so 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 = Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", false)
-            if ignore_hash
-                msg *= "\n\$JULIA_PKG_IGNORE_HASHES is set to 1: ignoring error and moving artifact to the expected location"
-                @error(msg)
-                # Move it to the location we expected
-                src = artifact_path(calc_hash; honor_overrides=false)
-                dst = artifact_path(tree_hash; honor_overrides=false)
-                mv(src, dst; force=true)
-                return true
-            end
-            return ErrorException(msg)
+    # Did we get what we expected?  If not, freak out.
+    if calc_hash.bytes != tree_hash.bytes
+        msg  = "Tree Hash Mismatch!\n"
+        msg *= "  Expected git-tree-sha1:   $(bytes2hex(tree_hash.bytes))\n"
+        msg *= "  Calculated git-tree-sha1: $(bytes2hex(calc_hash.bytes))"
+        # Since tree hash calculation is still broken on some systems, e.g. Pkg.jl#1860,
+        # and Pkg.jl#2317, 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 = Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", false)
+        if ignore_hash
+            msg *= "\n\$JULIA_PKG_IGNORE_HASHES is set to 1: ignoring error and moving artifact to the expected location"
+            @error(msg)
+            # Move it to the location we expected
+            src = artifact_path(calc_hash; honor_overrides=false)
+            dst = artifact_path(tree_hash; honor_overrides=false)
+            mv(src, dst; force=true)
+            return true
         end
+        return ErrorException(msg)
     end
 
     return true

src/utils.jl

@@ -46,6 +46,11 @@ function set_readonly(path)
             # outside of the root, links to non-file/non-directories, etc...)
             islink(filepath) && continue
             fmode = filemode(filepath)
+            @static if Sys.iswindows()
+                if Sys.isexecutable(filepath)
+                    fmode |= 0o111
+                end
+            end
             try
                 chmod(filepath, fmode & (typemax(fmode) ⊻ 0o222))
             catch

test/artifacts.jl

@@ -117,9 +117,7 @@ end
         @test artifact_exists(hash)
 
         # Test that the artifact verifies
-        if !Sys.iswindows()
-            @test verify_artifact(hash)
-        end
+        @test verify_artifact(hash)
     end
 
     @testset "File permissions" begin
@@ -190,9 +188,7 @@ end
                 @test !artifact_exists(arty_hash)
 
                 @test ensure_artifact_installed("arty", artifacts_toml) == artifact_path(arty_hash)
-                if !Sys.iswindows()
-                    @test verify_artifact(arty_hash)
-                end
+                @test verify_artifact(arty_hash)
 
                 # Make sure doing it twice "just works"
                 @test ensure_artifact_installed("arty", artifacts_toml) == artifact_path(arty_hash)
@@ -274,22 +270,20 @@ end
     @test_logs (:error, r"malformed, must be array or dict!") artifact_meta("broken_artifact", joinpath(badifact_dir, "not_a_table.toml"))
 
     # Next, test incorrect download errors
-    if !Sys.iswindows()
-        for ignore_hash in (false, true); withenv("JULIA_PKG_IGNORE_HASHES" => ignore_hash ? "1" : nothing) do; mktempdir() do dir
-            with_artifacts_directory(dir) do
-                @test artifact_meta("broken_artifact", joinpath(badifact_dir, "incorrect_gitsha.toml")) != nothing
-                if !ignore_hash
-                    @test_throws ErrorException ensure_artifact_installed("broken_artifact", joinpath(badifact_dir, "incorrect_gitsha.toml"))
-                else
-                    @test_logs (:error, r"Tree Hash Mismatch!") match_mode=:any  begin
-                        path = ensure_artifact_installed("broken_artifact", joinpath(badifact_dir, "incorrect_gitsha.toml"))
-                        @test endswith(path, "0000000000000000000000000000000000000000")
-                        @test isdir(path)
+    for ignore_hash in (false, true); withenv("JULIA_PKG_IGNORE_HASHES" => ignore_hash ? "1" : nothing) do; mktempdir() do dir
+        with_artifacts_directory(dir) do
+            @test artifact_meta("broken_artifact", joinpath(badifact_dir, "incorrect_gitsha.toml")) != nothing
+            if !ignore_hash
+                @test_throws ErrorException ensure_artifact_installed("broken_artifact", joinpath(badifact_dir, "incorrect_gitsha.toml"))
+            else
+                @test_logs (:error, r"Tree Hash Mismatch!") match_mode=:any  begin
+                    path = ensure_artifact_installed("broken_artifact", joinpath(badifact_dir, "incorrect_gitsha.toml"))
+                    @test endswith(path, "0000000000000000000000000000000000000000")
+                    @test isdir(path)
                     end
                 end
             end
-        end end end
-    end
+    end end end
 
     mktempdir() do dir
         with_artifacts_directory(dir) do