From ed49f139e583c537c6e2f727f0ad28f5824d63c7 Mon Sep 17 00:00:00 2001 From: Elliot Saba Date: Fri, 4 Sep 2020 10:58:31 -0700 Subject: [PATCH] Address review comments --- base/binaryplatforms.jl | 131 +++++++++++++++--------------- stdlib/Artifacts/src/Artifacts.jl | 42 ++++++---- 2 files changed, 91 insertions(+), 82 deletions(-) diff --git a/base/binaryplatforms.jl b/base/binaryplatforms.jl index 365a67360cc7c..00547dddd10e5 100644 --- a/base/binaryplatforms.jl +++ b/base/binaryplatforms.jl @@ -149,16 +149,16 @@ end # Simple equality definition; for compatibility testing, use `platforms_match()` Base.:(==)(a::AbstractPlatform, b::AbstractPlatform) = tags(a) == tags(b) +const ARCHITECTURE_FLAGS = Dict( + "x86_64" => ["x86_64", "avx", "avx2", "avx512"], + "i686" => ["prescott"], + "armv7l" => ["armv7l", "neon", "vfp4"], + "armv6l" => ["generic"], + "aarch64" => ["armv8", "thunderx2", "carmel"], + "powerpc64le" => ["generic"], +) function validate_tags(tags::Dict) throw_invalid_key(k) = throw(ArgumentError("Key \"$(k)\" cannot have value \"$(tags[k])\"")) - ARCHITECTURE_FLAGS = Dict( - "x86_64" => ["x86_64", "avx", "avx2", "avx512"], - "i686" => ["prescott"], - "armv7l" => ["armv7l", "neon", "vfp4"], - "armv6l" => ["generic"], - "aarch64" => ["armv8", "thunderx2", "carmel"], - "powerpc64le" => ["generic"], - ) # Validate `arch` if tags["arch"] ∉ keys(ARCHITECTURE_FLAGS) throw_invalid_key("arch") @@ -392,6 +392,15 @@ julia> call_Abi(Platform("x86_64", "macos")) ``` """ call_abi(p::AbstractPlatform) = get(tags(p), "call_abi", nothing) + + +const platform_namess = Dict( + "linux" => "Linux", + "macos" => "macOS", + "windows" => "Windows", + "freebsd" => "FreeBSD", + nothing => "Unknown", +) """ platform_name(p::AbstractPlatform) @@ -399,13 +408,6 @@ call_abi(p::AbstractPlatform) = get(tags(p), "call_abi", nothing) Get the "platform name" of the given platform, returning e.g. "Linux" or "Windows". """ function platform_name(p::AbstractPlatform) - names = Dict( - "linux" => "Linux", - "macos" => "macOS", - "windows" => "Windows", - "freebsd" => "FreeBSD", - nothing => "Unknown", - ) return names[os(p)] end @@ -553,6 +555,47 @@ Sys.islinux(p::AbstractPlatform) = os(p) == "linux" Sys.iswindows(p::AbstractPlatform) = os(p) == "windows" Sys.isfreebsd(p::AbstractPlatform) = os(p) == "freebsd" Sys.isbsd(p::AbstractPlatform) = os(p) ∈ ("freebsd", "macos") + +const arch_mapping = Dict( + "x86_64" => "(x86_|amd)64", + "i686" => "i\\d86", + "aarch64" => "(aarch64|arm64)", + "armv7l" => "arm(v7l)?", # if we just see `arm-linux-gnueabihf`, we assume it's `armv7l` + "armv6l" => "armv6l", + "powerpc64le" => "p(ower)?pc64le", +) +const os_mapping = Dict( + "macos" => "-apple-darwin[\\d\\.]*", + "freebsd" => "-(.*-)?freebsd[\\d\\.]*", + "windows" => "-w64-mingw32", + "linux" => "-(.*-)?linux", +) +const libc_mapping = Dict( + "libc_nothing" => "", + "glibc" => "-gnu", + "musl" => "-musl", +) +const call_abi_mapping = Dict( + "call_abi_nothing" => "", + "eabihf" => "eabihf", + "eabi" => "eabi", +) +const libgfortran_version_mapping = Dict( + "libgfortran_nothing" => "", + "libgfortran3" => "(-libgfortran3)|(-gcc4)", # support old-style `gccX` versioning + "libgfortran4" => "(-libgfortran4)|(-gcc7)", + "libgfortran5" => "(-libgfortran5)|(-gcc8)", +) +const libstdcxx_version_mapping = Dict{String,String}( + "libstdcxx_nothing" => "", + # This is sadly easier than parsing out the digit directly + ("libstdcxx$(idx)" => "-libstdcxx$(idx)" for idx in 18:26)..., +) +const cxxstring_abi_mapping = Dict( + "cxxstring_nothing" => "", + "cxx03" => "-cxx03", + "cxx11" => "-cxx11", +) """ parse(::Type{Platform}, triplet::AbstractString) @@ -560,52 +603,11 @@ Sys.isbsd(p::AbstractPlatform) = os(p) ∈ ("freebsd", "macos") Parses a string platform triplet back into a `Platform` object. """ function Base.parse(::Type{Platform}, triplet::AbstractString; validate_strict::Bool = false) - # We're going to build a mondo regex here to parse everything: - arch_mapping = Dict( - "x86_64" => "(x86_|amd)64", - "i686" => "i\\d86", - "aarch64" => "(aarch64|arm64)", - "armv7l" => "arm(v7l)?", # if we just see `arm-linux-gnueabihf`, we assume it's `armv7l` - "armv6l" => "armv6l", - "powerpc64le" => "p(ower)?pc64le", - ) - os_mapping = Dict( - "macos" => "-apple-darwin[\\d\\.]*", - "freebsd" => "-(.*-)?freebsd[\\d\\.]*", - "windows" => "-w64-mingw32", - "linux" => "-(.*-)?linux", - ) - libc_mapping = Dict( - "libc_nothing" => "", - "glibc" => "-gnu", - "musl" => "-musl", - ) - call_abi_mapping = Dict( - "call_abi_nothing" => "", - "eabihf" => "eabihf", - "eabi" => "eabi", - ) - libgfortran_version_mapping = Dict( - "libgfortran_nothing" => "", - "libgfortran3" => "(-libgfortran3)|(-gcc4)", # support old-style `gccX` versioning - "libgfortran4" => "(-libgfortran4)|(-gcc7)", - "libgfortran5" => "(-libgfortran5)|(-gcc8)", - ) - libstdcxx_version_mapping = Dict{String,String}( - "libstdcxx_nothing" => "", - # This is sadly easier than parsing out the digit directly - ("libstdcxx$(idx)" => "-libstdcxx$(idx)" for idx in 18:26)..., - ) - cxxstring_abi_mapping = Dict( - "cxxstring_nothing" => "", - "cxx03" => "-cxx03", - "cxx11" => "-cxx11", - ) - # Helper function to collapse dictionary of mappings down into a regex of # named capture groups joined by "|" operators c(mapping) = string("(",join(["(?<$k>$v)" for (k, v) in mapping], "|"), ")") + # We're going to build a mondo regex here to parse everything: triplet_regex = Regex(string( "^", # First, the core triplet; arch/os/libc/call_abi @@ -705,13 +707,13 @@ function Base.tryparse(::Type{Platform}, triplet::AbstractString) end """ - platform_dlext(p::AbstractPlatform = Platform()) + platform_dlext(p::AbstractPlatform = HostPlatform()) Return the dynamic library extension for the given platform, defaulting to the currently running platform. E.g. returns "so" for a Linux-based platform, "dll" for a Windows-based platform, etc... """ -function platform_dlext(p::AbstractPlatform = Platform()) +function platform_dlext(p::AbstractPlatform = HostPlatform()) if os(p) == "windows" return "dll" elseif os(p) == "macos" @@ -891,18 +893,17 @@ function host_triplet() return str end -# Cache the host platform value, and return it if someone asks for just `Platform()`. +# Cache the host platform value, and return it if someone asks for just `HostPlatform()`. default_host_platform = HostPlatform(parse(Platform, host_triplet())) """ - Platform() + HostPlatform() Return the `Platform` object that corresponds to the current host system, with all relevant comparison strategies set to host platform mode. This is equivalent to: HostPlatform(parse(Platform, Base.BinaryPlatforms.host_triplet())) """ -function Platform() - global default_host_platform +function HostPlatform() return default_host_platform::Platform end @@ -969,7 +970,7 @@ end platforms_match(a::AbstractString, b::AbstractString) = platforms_match(parse(Platform, a), parse(Platform, b)) """ - select_platform(download_info::Dict, platform::AbstractPlatform = Platform()) + select_platform(download_info::Dict, platform::AbstractPlatform = HostPlatform()) Given a `download_info` dictionary mapping platforms to some value, choose the value whose key best matches `platform`, returning `nothing` if no matches @@ -979,7 +980,7 @@ Platform attributes such as architecture, libc, calling ABI, etc... must all match exactly, however attributes such as compiler ABI can have wildcards within them such as `nothing` which matches any version of GCC. """ -function select_platform(download_info::Dict, platform::AbstractPlatform = Platform()) +function select_platform(download_info::Dict, platform::AbstractPlatform = HostPlatform()) ps = collect(filter(p -> platforms_match(p, platform), keys(download_info))) if isempty(ps) diff --git a/stdlib/Artifacts/src/Artifacts.jl b/stdlib/Artifacts/src/Artifacts.jl index d8e8d5fd12f5d..490926466e4d0 100644 --- a/stdlib/Artifacts/src/Artifacts.jl +++ b/stdlib/Artifacts/src/Artifacts.jl @@ -40,18 +40,18 @@ function with_artifacts_directory(f::Function, artifacts_dir::String) end """ - artifacts_dirs(args...) + artifacts_dirs(arg) Return a list of paths joined into all possible artifacts directories, as dictated by the current set of depot paths and the current artifact directory override via the method `with_artifacts_dir()`. """ -function artifacts_dirs(args...) +function artifacts_dirs(arg) if ARTIFACTS_DIR_OVERRIDE[] === nothing - return String[abspath(depot, "artifacts", args...) for depot in Base.DEPOT_PATH] + return String[abspath(depot, "artifacts", args) for depot in Base.DEPOT_PATH] else # If we've been given an override, use _only_ that directory. - return String[abspath(ARTIFACTS_DIR_OVERRIDE[], args...)] + return String[abspath(ARTIFACTS_DIR_OVERRIDE[], args)] end end @@ -104,11 +104,9 @@ function load_overrides(;force::Bool = false) function parse_mapping(mapping::String, name::String) if !isabspath(mapping) && !isempty(mapping) - try - mapping = Base.SHA1(mapping) - catch e + mapping = tryparse(Base.SHA1, mapping) + if mapping === nothing @error("Invalid override in '$(override_file)': entry '$(name)' must map to an absolute path or SHA1 hash!") - rethrow() end end return mapping @@ -119,9 +117,8 @@ function load_overrides(;force::Bool = false) for (k, mapping) in depot_override_dict # First, parse the mapping. Is it an absolute path, a valid SHA1-hash, or neither? - try - mapping = parse_mapping(mapping, k) - catch + mapping = parse_mapping(mapping, k) + if mapping === nothing @error("Invalid override in '$(override_file)': failed to parse entry `$(k)`") continue end @@ -129,9 +126,19 @@ function load_overrides(;force::Bool = false) # Next, determine if this is a hash override or a UUID/name override if isa(mapping, String) || isa(mapping, SHA1) # if this mapping is a direct mapping (e.g. a String), store it as a hash override - hash = try - Base.SHA1(hex2bytes(k)) - catch + local hash_str + try + hash_str = hex2bytes(k) + catch e + if isa(e, InterruptException) + rethrow(e) + end + @error("Invalid override in '$(override_file)': Invalid SHA1 hash '$(k)'") + continue + end + + hash = tryparse(Base.SHA1, hash_str) + if hash === nothing @error("Invalid override in '$(override_file)': Invalid SHA1 hash '$(k)'") continue end @@ -144,9 +151,8 @@ function load_overrides(;force::Bool = false) end elseif isa(mapping, Dict{String, Any}) # Convert `k` into a uuid - uuid = try - Base.UUID(k) - catch + uuid = tryparse(UUID, k) + if uuid === nothing @error("Invalid override in '$(override_file)': Invalid UUID '$(k)'") continue end @@ -167,6 +173,8 @@ function load_overrides(;force::Bool = false) ovruuid[uuid][name] = override_value end end + else + @error("Invalid override in '$(override_file)': unknown mapping type for '$(k)'") end end end