Skip to content

Commit

Permalink
Artifacts: avoid reflection hacks to access ensure_artifact_installed
Browse files Browse the repository at this point in the history
Packages should never access Base.loaded_modules() to call functions
from it, so instead we require the user to explicitly provide this
function, if they wish to use it (for lazy artifacts).
  • Loading branch information
vtjnash committed Oct 28, 2020
1 parent 0f0bc29 commit 6552d7a
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 43 deletions.
17 changes: 11 additions & 6 deletions stdlib/Artifacts/src/Artifacts.jl
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,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(ensure_artifact_installed))
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)
Expand All @@ -496,8 +496,10 @@ function _artifact_str(__module__, artifacts_toml, name, path_tail, artifact_dic

# 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 ensure_artifact_installed === nothing
error("Artifact $(repr(name)) was not installed correctly. Try `using Pkg; Pkg.build(\"$__module__\"); Pkg.Artifacts.ensure_all_artifacts_installed($(repr(artifacts_toml)))`?")
end
return jointail(ensure_artifact_installed(string(name), artifacts_toml; platform), path_tail)
end

"""
Expand Down Expand Up @@ -605,14 +607,17 @@ 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 run `using Pkg.Artifacts: ensure_artifact_installed`, and thus supports lazy artifacts
ensure_artifact_installed = isdefined(__module__, :ensure_artifact_installed) ? GlobalRef(__module__, :ensure_artifact_installed) : nothing

# 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), $(ensure_artifact_installed))
end
else
if platform === nothing
Expand All @@ -621,7 +626,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, $(ensure_artifact_installed))
end
end
end
Expand Down
1 change: 1 addition & 0 deletions stdlib/Artifacts/test/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/artifacts/
24 changes: 24 additions & 0 deletions stdlib/Artifacts/test/refresh_artifacts.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
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")
rm(tempdir; recursive=true, force=true)
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
80 changes: 44 additions & 36 deletions stdlib/Artifacts/test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
using Artifacts, Test, Base.BinaryPlatforms
using Artifacts: with_artifacts_directory, pack_platform!, unpack_platform

run(`$(Base.julia_cmd()) $(joinpath(@__DIR__, "refresh_artifacts.jl"))`)

@testset "Artifact Paths" begin
mktempdir() do tempdir
with_artifacts_directory(tempdir) do
Expand Down Expand Up @@ -78,44 +80,50 @@ 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
end

@testset "@artifact_str lazy" begin
tempdir = joinpath(@__DIR__, "artifacts")
with_artifacts_directory(tempdir) do
ex = @test_throws ErrorException artifact"socrates"
@test startswith(ex.value.msg, "Artifact \"socrates\" was not installed correctly.")
end
end
2 changes: 1 addition & 1 deletion test/choosetests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,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"]
net_on = true
try
ipa = getipaddr()
Expand Down

0 comments on commit 6552d7a

Please sign in to comment.