From 7d5dbf56b298ece51eaea9d410a3760b8a9f24ba Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Mon, 23 Nov 2020 12:56:13 -0500 Subject: [PATCH] Artifacts: avoid reflection hacks to access ensure_artifact_installed (#37844) 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. --- NEWS.md | 2 + base/sysimg.jl | 3 + stdlib/Artifacts/src/Artifacts.jl | 42 +++++++---- stdlib/Artifacts/test/.gitignore | 1 + stdlib/Artifacts/test/refresh_artifacts.jl | 23 ++++++ stdlib/Artifacts/test/runtests.jl | 82 +++++++++++++--------- stdlib/LazyArtifacts/Project.toml | 12 ++++ stdlib/LazyArtifacts/src/LazyArtifacts.jl | 15 ++++ stdlib/LazyArtifacts/test/Artifacts.toml | 1 + stdlib/LazyArtifacts/test/runtests.jl | 25 +++++++ stdlib/Makefile | 2 +- test/choosetests.jl | 2 +- test/precompile.jl | 2 +- 13 files changed, 161 insertions(+), 51 deletions(-) create mode 100644 stdlib/Artifacts/test/.gitignore create mode 100644 stdlib/Artifacts/test/refresh_artifacts.jl create mode 100644 stdlib/LazyArtifacts/Project.toml create mode 100644 stdlib/LazyArtifacts/src/LazyArtifacts.jl create mode 120000 stdlib/LazyArtifacts/test/Artifacts.toml create mode 100644 stdlib/LazyArtifacts/test/runtests.jl diff --git a/NEWS.md b/NEWS.md index 3188710d4322a..aea361ad659aa 100644 --- 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 index f15e402787cb8..4de093d640555 100644 --- 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 index 3b99a8d23c28c..50f36fe53a954 100644 --- 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 new file mode 100644 index 0000000000000..37beecae3371b --- /dev/null +++ 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 new file mode 100644 index 0000000000000..b8de741c93b7c --- /dev/null +++ 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 index ce0a2ceb654de..5e89f97bfba35 100644 --- 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 new file mode 100644 index 0000000000000..ea9afc9d12dba --- /dev/null +++ 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 new file mode 100644 index 0000000000000..b783276ac6081 --- /dev/null +++ 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 new file mode 120000 index 0000000000000..1b01a83fcf079 --- /dev/null +++ b/stdlib/LazyArtifacts/test/Artifacts.toml @@ -0,0 +1 @@ +../../Artifacts/test/Artifacts.toml \ No newline at end of file diff --git a/stdlib/LazyArtifacts/test/runtests.jl b/stdlib/LazyArtifacts/test/runtests.jl new file mode 100644 index 0000000000000..8cdfacc595d44 --- /dev/null +++ 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 index 810c365347881..02152127f7b41 100644 --- 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 index 914b199a7a852..460b8121bb9f5 100644 --- 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 index 217bf2eae6279..6f1487fcbb484 100644 --- 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,