Artifacts: avoid reflection hacks to access ensure_artifact_installed

Packages should never access Base.loaded_modules() to call functions from it, as it can be brittle and create future incompatibilities, so instead we require the user to explicitly declare a dependency on the lazy-download machinery, if they requiring the ability to use it (for lazy artifacts). As a deprecation, if the user has `using Pkg`, that will be used instead, with a depwarn.
JuliaLang · Nov 20, 2020 · 2ef080e · 2ef080e
1 parent c36cf8c
commit 2ef080e
Show file tree

Hide file tree

Showing 13 changed files with 161 additions and 51 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -150,6 +150,8 @@ Standard library changes
 * The `Pkg.Artifacts` module has been imported as a separate standard library.  It is still available as
   `Pkg.Artifacts`, however starting from Julia v1.6+, packages may import simply `Artifacts` without importing
   all of `Pkg` alongside ([#37320]).
+* To download artifacts lazily, `LazyArtifacts` now must be explicitly listed as a dependency, to avoid needing the
+  support machinery to be available when it is not commonly needed ([#37844]).
 * `@time` now reports if the time presented included any compilation time, which is shown as a percentage ([#37678]).
 * `@varinfo` can now report non-exported objects within modules, look recursively into submodules, and return a sorted
   results table ([#38042]).

diff --git a/base/sysimg.jl b/base/sysimg.jl
@@ -68,6 +68,9 @@ let
 
         # 4-depth packages
         :Pkg,
+
+        # 5-depth packages
+        :LazyArtifacts,
     ]
     maxlen = reduce(max, textwidth.(string.(stdlibs)); init=0)
 

diff --git a/stdlib/Artifacts/src/Artifacts.jl b/stdlib/Artifacts/src/Artifacts.jl
@@ -524,7 +524,7 @@ function jointail(dir, tail)
     end
 end
 
-function _artifact_str(__module__, artifacts_toml, name, path_tail, artifact_dict, hash, platform)
+function _artifact_str(__module__, artifacts_toml, name, path_tail, artifact_dict, hash, platform, @nospecialize(lazyartifacts))
     if haskey(Base.module_keys, __module__)
         # Process overrides for this UUID, if we know what it is
         process_overrides(artifact_dict, Base.module_keys[__module__].uuid)
@@ -538,10 +538,16 @@ function _artifact_str(__module__, artifacts_toml, name, path_tail, artifact_dic
         end
     end
 
-    # If not, we need to download it.  We look up the Pkg module through `Base.loaded_modules()`
-    # then invoke `ensure_artifact_installed()`:
-    Pkg = first(filter(p-> p[1].name == "Pkg", Base.loaded_modules))[2]
-    return jointail(Pkg.Artifacts.ensure_artifact_installed(string(name), artifacts_toml; platform), path_tail)
+    # If not, try determining what went wrong:
+    meta = artifact_meta(name, artifact_dict, artifacts_toml; platform)
+    if meta !== nothing && get(meta, "lazy", false)
+        if lazyartifacts isa Module && isdefined(lazyartifacts, :ensure_artifact_installed)
+            nameof(lazyartifacts) === :Pkg && Base.depwarn("using Pkg instead of using LazyArtifacts is deprecated", :var"@artifact_str", force=true)
+            return jointail(lazyartifacts.ensure_artifact_installed(string(name), artifacts_toml; platform), path_tail)
+        end
+        error("Artifact $(repr(name)) is a lazy artifact; package developers must call `using LazyArtifacts` in $(__module__) before using lazy artifacts.")
+    end
+    error("Artifact $(repr(name)) was not installed correctly. Try `using Pkg; Pkg.instantiate()` to re-install all missing resources.")
 end
 
 """
@@ -607,11 +613,15 @@ end
 """
     macro artifact_str(name)
 
-Macro that is used to automatically ensure an artifact is installed, and return its
-location on-disk.  Automatically looks the artifact up by name in the project's
-`(Julia)Artifacts.toml` file.  Throws an error on inability to install the requested
-artifact.  If run in the REPL, searches for the toml file starting in the current
-directory, see `find_artifacts_toml()` for more.
+Return the on-disk path to an artifact. Automatically looks the artifact up by
+name in the project's `(Julia)Artifacts.toml` file. Throws an error on if the
+requested artifact is not present. If run in the REPL, searches for the toml
+file starting in the current directory, see `find_artifacts_toml()` for more.
+
+If the artifact is marked "lazy" and the package has `using LazyArtifacts`
+defined, the artifact will be downloaded on-demand with `Pkg` the first time
+this macro tries to compute the path. The files will then be left installed
+locally for later.
 
 If `name` contains a forward or backward slash, all elements after the first slash will
 be taken to be path names indexing into the artifact, allowing for an easy one-liner to
@@ -649,14 +659,20 @@ macro artifact_str(name, platform=nothing)
     # Invalidate calling .ji file if Artifacts.toml file changes
     Base.include_dependency(artifacts_toml)
 
+    # Check if the user has provided `LazyArtifacts`, and thus supports lazy artifacts
+    lazyartifacts = isdefined(__module__, :LazyArtifacts) ? GlobalRef(__module__, :LazyArtifacts) : nothing
+    if lazyartifacts === nothing && isdefined(__module__, :Pkg)
+        lazyartifacts = GlobalRef(__module__, :Pkg) # deprecated
+    end
+
     # If `name` is a constant, (and we're using the default `Platform`) we can actually load
     # and parse the `Artifacts.toml` file now, saving the work from runtime.
     if isa(name, AbstractString) && platform === nothing
         # To support slash-indexing, we need to split the artifact name from the path tail:
         platform = HostPlatform()
-        local artifact_name, artifact_path_tail, hash = artifact_slash_lookup(name, artifact_dict, artifacts_toml, platform)
+        artifact_name, artifact_path_tail, hash = artifact_slash_lookup(name, artifact_dict, artifacts_toml, platform)
         return quote
-            Base.invokelatest(_artifact_str, $(__module__), $(artifacts_toml), $(artifact_name), $(artifact_path_tail), $(artifact_dict), $(hash), $(platform))
+            Base.invokelatest(_artifact_str, $(__module__), $(artifacts_toml), $(artifact_name), $(artifact_path_tail), $(artifact_dict), $(hash), $(platform), $(lazyartifacts))
         end
     else
         if platform === nothing
@@ -665,7 +681,7 @@ macro artifact_str(name, platform=nothing)
         return quote
             local platform = $(esc(platform))
             local artifact_name, artifact_path_tail, hash = artifact_slash_lookup($(esc(name)), $(artifact_dict), $(artifacts_toml), platform)
-            Base.invokelatest(_artifact_str, $(__module__), $(artifacts_toml), artifact_name, artifact_path_tail, $(artifact_dict), hash, platform)
+            Base.invokelatest(_artifact_str, $(__module__), $(artifacts_toml), artifact_name, artifact_path_tail, $(artifact_dict), hash, platform, $(lazyartifacts))
         end
     end
 end

diff --git a/stdlib/Artifacts/test/.gitignore b/stdlib/Artifacts/test/.gitignore
@@ -0,0 +1 @@
+/artifacts/
diff --git a/stdlib/Artifacts/test/refresh_artifacts.jl b/stdlib/Artifacts/test/refresh_artifacts.jl
@@ -0,0 +1,23 @@
+using Artifacts: with_artifacts_directory
+# using Pkg.Artifacts: ensure_all_artifacts_installed
+using Pkg.Artifacts: load_artifacts_toml, ensure_artifact_installed
+let
+    tempdir = joinpath(@__DIR__, "artifacts")
+    toml = joinpath(@__DIR__, "Artifacts.toml")
+    unused = Base.BinaryPlatforms.Platform(string(Sys.ARCH), "linux")
+    with_artifacts_directory(tempdir) do
+        # ensure_all_artifacts_installed(toml; include_lazy=false)
+        dict = load_artifacts_toml(toml)
+        for (name, meta) in dict
+            if meta isa Array
+                for meta in meta
+                    get(meta, "lazy", false) && continue
+                    ensure_artifact_installed(name, meta, toml; platform=unused)
+                end
+            else; meta::Dict
+                get(meta, "lazy", false) && continue
+                ensure_artifact_installed(name, meta, toml; platform=unused)
+            end
+        end
+    end
+end
diff --git a/stdlib/Artifacts/test/runtests.jl b/stdlib/Artifacts/test/runtests.jl
@@ -3,6 +3,9 @@
 using Artifacts, Test, Base.BinaryPlatforms
 using Artifacts: with_artifacts_directory, pack_platform!, unpack_platform
 
+# prepare for the package tests by ensuring the required artifacts are downloaded now
+run(addenv(`$(Base.julia_cmd()) --color=no $(joinpath(@__DIR__, "refresh_artifacts.jl"))`, "TERM"=>"dumb"))
+
 @testset "Artifact Paths" begin
     mktempdir() do tempdir
         with_artifacts_directory(tempdir) do
@@ -78,45 +81,43 @@ end
 end
 
 @testset "Artifact Slash-indexing" begin
-    mktempdir() do tempdir
-        with_artifacts_directory(tempdir) do
-            exeext = Sys.iswindows() ? ".exe" : ""
-
-            # simple lookup, gives us the directory for `c_simple` for the current architecture
-            c_simple_dir = artifact"c_simple"
-            @test isdir(c_simple_dir)
-            c_simple_exe_path = joinpath(c_simple_dir, "bin", "c_simple$(exeext)")
-            @test isfile(c_simple_exe_path)
-
-            # Simple slash-indexed lookup
-            c_simple_bin_path = artifact"c_simple/bin"
-            @test isdir(c_simple_bin_path)
-            # Test that forward and backward slash are equivalent
-            @test artifact"c_simple\\bin" == artifact"c_simple/bin"
-
-            # Dynamically-computed lookup; not done at compile-time
-            generate_artifact_name() = "c_simple"
-            c_simple_dir = @artifact_str(generate_artifact_name())
-            @test isdir(c_simple_dir)
-            c_simple_exe_path = joinpath(c_simple_dir, "bin", "c_simple$(exeext)")
-            @test isfile(c_simple_exe_path)
-
-            # Dynamically-computed slash-indexing:
-            generate_bin_path(pathsep) = "c_simple$(pathsep)bin$(pathsep)c_simple$(exeext)"
-            @test isfile(@artifact_str(generate_bin_path("/")))
-            @test isfile(@artifact_str(generate_bin_path("\\")))
-        end
+    tempdir = joinpath(@__DIR__, "artifacts")
+    with_artifacts_directory(tempdir) do
+        exeext = Sys.iswindows() ? ".exe" : ""
+
+        # simple lookup, gives us the directory for `c_simple` for the current architecture
+        c_simple_dir = artifact"c_simple"
+        @test isdir(c_simple_dir)
+        c_simple_exe_path = joinpath(c_simple_dir, "bin", "c_simple$(exeext)")
+        @test isfile(c_simple_exe_path)
+
+        # Simple slash-indexed lookup
+        c_simple_bin_path = artifact"c_simple/bin"
+        @test isdir(c_simple_bin_path)
+        # Test that forward and backward slash are equivalent
+        @test artifact"c_simple\\bin" == artifact"c_simple/bin"
+
+        # Dynamically-computed lookup; not done at compile-time
+        generate_artifact_name() = "c_simple"
+        c_simple_dir = @artifact_str(generate_artifact_name())
+        @test isdir(c_simple_dir)
+        c_simple_exe_path = joinpath(c_simple_dir, "bin", "c_simple$(exeext)")
+        @test isfile(c_simple_exe_path)
+
+        # Dynamically-computed slash-indexing:
+        generate_bin_path(pathsep) = "c_simple$(pathsep)bin$(pathsep)c_simple$(exeext)"
+        @test isfile(@artifact_str(generate_bin_path("/")))
+        @test isfile(@artifact_str(generate_bin_path("\\")))
     end
 end
 
 @testset "@artifact_str Platform passing" begin
-    mktempdir() do tempdir
-        with_artifacts_directory(tempdir) do
-            win64 = Platform("x86_64", "windows")
-            mac64 = Platform("x86_64", "macos")
-            @test basename(@artifact_str("c_simple", win64)) == "444cecb70ff39e8961dd33e230e151775d959f37"
-            @test basename(@artifact_str("c_simple", mac64)) == "7ba74e239348ea6c060f994c083260be3abe3095"
-        end
+    tempdir = joinpath(@__DIR__, "artifacts")
+    with_artifacts_directory(tempdir) do
+        win64 = Platform("x86_64", "windows")
+        mac64 = Platform("x86_64", "macos")
+        @test basename(@artifact_str("c_simple", win64)) == "444cecb70ff39e8961dd33e230e151775d959f37"
+        @test basename(@artifact_str("c_simple", mac64)) == "7ba74e239348ea6c060f994c083260be3abe3095"
     end
 end
 
@@ -131,3 +132,14 @@ end
     @test artifacts["c_simple"]["git-tree-sha1"] == "0c509b3302db90a9393d6036c3ffcd14d190523d"
     @test artifacts["socrates"]["git-tree-sha1"] == "43563e7631a7eafae1f9f8d9d332e3de44ad7239"
 end
+
+@testset "@artifact_str install errors" begin
+    mktempdir() do tempdir
+        with_artifacts_directory(tempdir) do
+            ex = @test_throws ErrorException artifact"c_simple"
+            @test startswith(ex.value.msg, "Artifact \"c_simple\" was not installed correctly. ")
+            ex = @test_throws ErrorException artifact"socrates"
+            @test startswith(ex.value.msg, "Artifact \"socrates\" is a lazy artifact; ")
+        end
+    end
+end
diff --git a/stdlib/LazyArtifacts/Project.toml b/stdlib/LazyArtifacts/Project.toml
@@ -0,0 +1,12 @@
+name = "LazyArtifacts"
+uuid = "4af54fe1-eca0-43a8-85a7-787d91b784e3"
+
+[deps]
+Artifacts = "56f22d72-fd6d-98f1-02f0-08ddc0907c33"
+Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
+
+[extras]
+Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
+
+[targets]
+test = ["Test"]
diff --git a/stdlib/LazyArtifacts/src/LazyArtifacts.jl b/stdlib/LazyArtifacts/src/LazyArtifacts.jl
@@ -0,0 +1,15 @@
+# This file is a part of Julia. License is MIT: https://julialang.org/license
+
+module LazyArtifacts
+
+# reexport the Artifacts API
+using Artifacts: Artifacts,
+       artifact_exists, artifact_path, artifact_meta, artifact_hash,
+       select_downloadable_artifacts, find_artifacts_toml, @artifact_str
+export artifact_exists, artifact_path, artifact_meta, artifact_hash,
+       select_downloadable_artifacts, find_artifacts_toml, @artifact_str
+
+# define a function for satisfying lazy Artifact downloads
+using Pkg.Artifacts: ensure_artifact_installed
+
+end
diff --git a/stdlib/LazyArtifacts/test/Artifacts.toml b/stdlib/LazyArtifacts/test/Artifacts.toml
@@ -0,0 +1 @@
+../../Artifacts/test/Artifacts.toml
diff --git a/stdlib/LazyArtifacts/test/runtests.jl b/stdlib/LazyArtifacts/test/runtests.jl
@@ -0,0 +1,25 @@
+using LazyArtifacts
+using Test
+
+mktempdir() do tempdir
+    LazyArtifacts.Artifacts.with_artifacts_directory(tempdir) do
+        socrates_dir = artifact"socrates"
+        @test isdir(socrates_dir)
+        ex = @test_throws ErrorException artifact"c_simple"
+        @test startswith(ex.value.msg, "Artifact \"c_simple\" was not installed correctly. ")
+    end
+end
+
+# Need to set depwarn flag before testing deprecations
+@test success(run(setenv(`$(Base.julia_cmd()) --depwarn=no --startup-file=no -e '
+    using Artifacts, Pkg
+    using Test
+    mktempdir() do tempdir
+        Artifacts.with_artifacts_directory(tempdir) do
+            socrates_dir = @test_logs(
+                    (:warn, "using Pkg instead of using LazyArtifacts is deprecated"),
+                    artifact"socrates")
+            @test isdir(socrates_dir)
+        end
+    end'`,
+    dir=@__DIR__)))
diff --git a/stdlib/Makefile b/stdlib/Makefile
@@ -15,7 +15,7 @@ $(build_datarootdir)/julia/stdlib/$(VERSDIR):
 	mkdir -p $@
 
 STDLIBS = Artifacts Base64 CRC32c Dates DelimitedFiles Distributed FileWatching \
-          Future InteractiveUtils Libdl LibGit2 LinearAlgebra Logging \
+          Future InteractiveUtils LazyArtifacts Libdl LibGit2 LinearAlgebra Logging \
           Markdown Mmap Printf Profile Random REPL Serialization SHA \
           SharedArrays Sockets SparseArrays SuiteSparse Test TOML Unicode UUIDs \
           MozillaCACerts_jll LibCURL_jll

diff --git a/test/choosetests.jl b/test/choosetests.jl
@@ -112,7 +112,7 @@ function choosetests(choices = [])
         filter!(x -> (x != "Profile"), tests)
     end
 
-    net_required_for = ["Sockets", "LibGit2", "LibCURL", "Downloads"]
+    net_required_for = ["Sockets", "LibGit2", "LibCURL", "Downloads", "Artifacts", "LazyArtifacts"]
     net_on = true
     try
         ipa = getipaddr()

diff --git a/test/precompile.jl b/test/precompile.jl
@@ -291,7 +291,7 @@ try
                      Base.PkgId(m) => Base.module_build_id(m)
                  end for s in
                 [:Artifacts, :Base64, :CRC32c, :Dates, :DelimitedFiles, :Distributed, :FileWatching, :Markdown,
-                 :Future, :Libdl, :LinearAlgebra, :Logging, :Mmap, :Printf,
+                 :Future, :LazyArtifacts, :Libdl, :LinearAlgebra, :Logging, :Mmap, :Printf,
                  :Profile, :Random, :Serialization, :SharedArrays, :SparseArrays, :SuiteSparse, :Test,
                  :Unicode, :REPL, :InteractiveUtils, :Pkg, :LibGit2, :SHA, :UUIDs, :Sockets,
                  :Statistics, :TOML, :MozillaCACerts_jll, :LibCURL_jll, :LibCURL, :Downloads,