Some fixes suggested by JET (#196)

Author: giordano

src/BinaryBuilderBase.jl

@@ -45,7 +45,7 @@ include("utils.jl")
 # downloads, unpacked .tar.gz shards, mounted shards, ccache cache, etc....
 function storage_dir(args::AbstractString...)
     global storage_cache
-    dir = joinpath(storage_cache, args...)
+    dir = joinpath(storage_cache[], args...)
     mkpath(dirname(dir))
     return dir
 end
@@ -64,11 +64,11 @@ enable_apple_file() = storage_dir("enable_apple")
 # These globals store important information such as where we're downloading
 # the rootfs to, and where we're unpacking it.  These constants are initialized
 # by `__init__()` to allow for environment variable overrides from the user.
-storage_cache = ""
-use_squashfs = false
-allow_ecryptfs = false
-use_ccache = false
-bootstrap_list = Symbol[]
+const storage_cache = Ref("")
+const use_squashfs = Ref(false)
+const allow_ecryptfs = Ref(false)
+const use_ccache = Ref(false)
+const bootstrap_list = Symbol[]
 
 function get_bbb_version(dir=@__DIR__, uuid="7f725544-6523-48cd-82d1-3fa08ff4056e")
     # Get BinaryBuilder.jl's version and git sha
@@ -154,7 +154,7 @@ function versioninfo(; name=@__MODULE__, version=get_bbb_version())
     run_interactive(runner, `/bin/bash -c "echo hello julia"`)
 
     # If we use ccache, dump the ccache stats
-    if use_ccache
+    if use_ccache[]
         @info("ccache stats:")
         runner = preferred_runner()(
             pwd();
@@ -171,8 +171,8 @@ function __init__()
     global use_ccache, storage_cache
 
     # Allow the user to override the default value for `storage_dir`
-    storage_cache = get(ENV, "BINARYBUILDER_STORAGE_DIR",
-                        abspath(joinpath(@__DIR__, "..", "deps")))
+    storage_cache[] = get(ENV, "BINARYBUILDER_STORAGE_DIR",
+                          abspath(joinpath(@__DIR__, "..", "deps")))
 
     # If the user has signalled that they really want us to automatically
     # accept apple EULAs, do that.
@@ -181,13 +181,13 @@ function __init__()
     end
 
     # If the user has overridden our runner selection algorithms, honor that
-    runner_override = lowercase(get(ENV, "BINARYBUILDER_RUNNER", ""))
-    if runner_override == "unprivileged"
-        runner_override = "userns"
+    runner_override[] = lowercase(get(ENV, "BINARYBUILDER_RUNNER", ""))
+    if runner_override[] == "unprivileged"
+        runner_override[] = "userns"
     end
-    if !(runner_override in ["", "userns", "privileged", "docker"])
-        @warn("Invalid runner value $runner_override, ignoring...")
-        runner_override = ""
+    if !(runner_override[] in ("", "userns", "privileged", "docker"))
+        @warn("Invalid runner value $(runner_override[]), ignoring...")
+        runner_override[] = ""
     end
 
     # If the user has asked for squashfs mounting instead of tarball mounting,
@@ -196,35 +196,30 @@ function __init__()
     # default. If we are not on Travis, we default to using tarballs and not
     # squashfs images as using them requires `sudo` access.
     if get(ENV, "BINARYBUILDER_USE_SQUASHFS", "") == "false"
-        use_squashfs = false
+        use_squashfs[] = false
     elseif get(ENV, "BINARYBUILDER_USE_SQUASHFS", "") == "true"
-        use_squashfs = true
+        use_squashfs[] = true
     else
-        # If it hasn't been specified, but we're on Travis, default to "on"
-        if get(ENV, "TRAVIS", "") == "true"
-            use_squashfs = true
-        end
-
         # If it hasn't been specified but we're going to use the docker runner,
         # then set `use_squashfs` to `true` here.
         if preferred_runner() == DockerRunner
             # Conversely, if we're dock'ing it up, don't use it.
-            use_squashfs = false
+            use_squashfs[] = false
         elseif runner_override == "privileged"
             # If we're forcing a privileged runner, go ahead and default to squashfs
-            use_squashfs = true
+            use_squashfs[] = true
         end
     end
 
     # If the user has signified that they want to allow mounting of ecryptfs
     # paths, then let them do so at their own peril.
     if get(ENV, "BINARYBUILDER_ALLOW_ECRYPTFS", "") == "true"
-        allow_ecryptfs = true
+        allow_ecryptfs[] = true
     end
 
     # If the user has enabled `ccache` support, use it!
     if get(ENV, "BINARYBUILDER_USE_CCACHE", "false") == "true"
-        use_ccache = true
+        use_ccache[] = true
     end
 end
 

src/BuildToolchains.jl

@@ -26,6 +26,8 @@ function cmake_os(p::AbstractPlatform)
         return "Darwin"
     elseif Sys.iswindows(p)
         return "Windows"
+    else
+        return "Unknown"
     end
 end
 
@@ -162,7 +164,7 @@ function meson_cpu_family(p::AbstractPlatform)
         return "x86_64"
     elseif arch(p) == "aarch64"
         return "aarch64"
-    elseif startswith(arch(p), "arm")
+    elseif startswith(arch(p)::String, "arm")
         return "arm"
     end
 end

src/DockerRunner.jl

@@ -91,7 +91,7 @@ function DockerRunner(workspace_root::String;
     # Construct docker command
     docker_cmd = `docker run --rm --privileged `#--cap-add SYS_ADMIN`
 
-    if cwd != nothing
+    if cwd !== nothing
         docker_cmd = `$docker_cmd -w /$(abspath(cwd))`
     end
 

src/Platforms.jl

@@ -25,8 +25,8 @@ Strip out any tags that are not the basic annotations like `libc` and `call_abi`
 """
 function abi_agnostic(p::Platform)
     keeps = ("libc", "call_abi", "os_version")
-    filtered_tags = Dict(Symbol(k) => v for (k, v) in tags(p) if k ∈ keeps)
-    return Platform(arch(p), os(p); filtered_tags...)
+    filtered_tags = Dict{Symbol,String}(Symbol(k) => v for (k, v) in tags(p) if k ∈ keeps)
+    return Platform(arch(p)::String, os(p)::String; filtered_tags...)
 end
 abi_agnostic(p::AnyPlatform) = p
 

src/Products.jl

@@ -51,7 +51,7 @@ If `isolate` is set to `true`, will isolate all checks from the main Julia
 process in the event that `dlopen()`'ing a library might cause issues.
 """
 function satisfied(p::Product, prefix::Prefix; kwargs...)
-    return locate(p, prefix; kwargs...) != nothing
+    return locate(p, prefix; kwargs...) !== nothing
 end
 
 
@@ -357,7 +357,7 @@ struct ExecutableProduct <: Product
             error("`$(varname)` is already defined in Base")
         end
         # If some other kind of AbstractString is passed in, convert it
-        if dir_path != nothing
+        if dir_path !== nothing
             dir_path = string(dir_path)
         end
         return new(binnames, varname, dir_path)
@@ -378,7 +378,7 @@ end
 function repr(p::ExecutableProduct)
     varname = repr(p.variable_name)
     binnames = repr(p.binnames)
-    if p.dir_path != nothing
+    if p.dir_path !== nothing
         return "ExecutableProduct($(binnames), $(varname), $(repr(p.dir_path)))"
     else
         return "ExecutableProduct($(binnames), $(varname))"
@@ -410,10 +410,10 @@ function locate(ep::ExecutableProduct, prefix::Prefix; platform::AbstractPlatfor
         end
 
         # Join into the `dir_path` given by the executable product
-        if ep.dir_path != nothing
-            path = joinpath(prefix.path, template(joinpath(ep.dir_path, binname), platform))
+        path = if ep.dir_path !== nothing
+            joinpath(prefix.path, template(joinpath(ep.dir_path, binname), platform))
         else
-            path = joinpath(bindir(prefix), template(binname, platform))
+            joinpath(bindir(prefix), template(binname, platform))
         end
 
         if isfile(path)

src/Rootfs.jl

@@ -39,8 +39,8 @@ struct CompilerShard
         # by higher-level things to choose e.g. which version of GCC
         # to use, but once we're at this level we only care about the
         # larger-scale things, not the ABI).
-        host = abi_agnostic(host)
-        if target != nothing
+        host = abi_agnostic(host::AbstractPlatform)
+        if target !== nothing
             target = abi_agnostic(target)
         end
 
@@ -64,8 +64,8 @@ Return the bound artifact name for a particular shard.
 """
 function artifact_name(cs::CompilerShard)
     target_str = ""
-    if cs.target != nothing
-        target_str = "-$(triplet(cs.target))"
+    if cs.target !== nothing
+        target_str = "-$(triplet(cs.target::Platform))"
 
         if cs.name in ("GCCBootstrap", "PlatformSupport")
             # armv6l uses the same GCC shards as armv7l, so we just rename here.
@@ -84,15 +84,15 @@ function CompilerShard(art_name::String)
     end
     return CompilerShard(
         m.captures[1],
-        VersionNumber(m.captures[3]),
+        VersionNumber(m.captures[3]::AbstractString),
         m.captures[4],
         Symbol(m.captures[5]);
         target=m.captures[2]
     )
 end
 
 const ALL_SHARDS = Ref{Union{Vector{CompilerShard},Nothing}}(nothing)
-function all_compiler_shards()
+function all_compiler_shards()::Vector{CompilerShard}
     if ALL_SHARDS[] === nothing
         artifacts_toml = joinpath(dirname(@__DIR__), "Artifacts.toml")
         artifact_dict = load_artifacts_toml(artifacts_toml)
@@ -114,13 +114,13 @@ function all_compiler_shards()
             end
 
             # If this compiler shard has an os_version, that should be interpreted as the bound it is.
-            if cs.target !== nothing && os_version(cs.target) !== nothing
-                set_compare_strategy!(cs.target, "os_version", compare_version_cap)
+            if cs.target !== nothing && os_version(cs.target::Platform) !== nothing
+                set_compare_strategy!(cs.target::Platform, "os_version", compare_version_cap)
             end
-            push!(ALL_SHARDS[], cs)
+            push!(ALL_SHARDS[]::Vector{CompilerShard}, cs)
         end
     end
-    return ALL_SHARDS[]
+    return ALL_SHARDS[]::Vector{CompilerShard}
 end
 
 function shard_source_artifact_hash(cs::CompilerShard)
@@ -168,7 +168,7 @@ function map_target(cs::CompilerShard)
         return "/"
     elseif lowercase(cs.name) == "rusttoolchain"
         # We override RustToolchain because they all have to sit in the same location
-        return "/opt/$(aatriplet(cs.host))/$(cs.name)-$(cs.version)-$(aatriplet(cs.target)))"
+        return "/opt/$(aatriplet(cs.host))/$(cs.name)-$(cs.version)-$(aatriplet(cs.target::Platform)))"
     else
         return joinpath("/opt", aatriplet(something(cs.target, cs.host)), "$(cs.name)-$(cs.version)")
     end
@@ -231,7 +231,7 @@ function mount(cs::CompilerShard, build_prefix::AbstractString; verbose::Bool =
     # they must accept the Xcode EULA.  This will be skipped if either the
     # environment variable BINARYBUILDER_AUTOMATIC_APPLE has been set to `true`
     # or if the SDK has been downloaded in the past.
-    if cs.target !== nothing && Sys.isapple(cs.target) && !isfile(enable_apple_file()) && !macos_sdk_already_installed()
+    if cs.target !== nothing && Sys.isapple(cs.target::Platform) && !isfile(enable_apple_file()) && !macos_sdk_already_installed()
         if !isinteractive()
             msg = strip("""
             This is not an interactive Julia session, so we will not prompt you
@@ -343,7 +343,7 @@ Returns `true` if any piece of the MacOS SDK is already installed.
 function macos_sdk_already_installed()
     # Get all compiler shards we know about
     css = all_compiler_shards()
-    macos_artifact_names = artifact_name.(filter(cs -> cs.target !== nothing && Sys.isapple(cs.target), css))
+    macos_artifact_names = artifact_name.(filter(cs -> cs.target !== nothing && Sys.isapple(cs.target::Platform), css))
 
     artifacts_toml = joinpath(dirname(@__DIR__), "Artifacts.toml")
     macos_artifact_hashes = artifact_hash.(macos_artifact_names, artifacts_toml; platform=default_host_platform)
@@ -481,7 +481,7 @@ end
 
 function llvm_version(p::AbstractPlatform, LLVM_builds::Vector{LLVMBuild})
     if march(p) in ("armv8_2_crypto", "armv8_4_crypto_sve")
-        LLVM_builds = filter(b -> getversion(b) >= v"9.0")
+        LLVM_builds = filter(b -> getversion(b) >= v"9.0", LLVM_builds)
     end
     return getversion.(LLVM_builds)
 end
@@ -527,7 +527,7 @@ function choose_shards(p::AbstractPlatform;
             LLVM_builds::Vector{LLVMBuild}=available_llvm_builds,
             Rust_build::VersionNumber=v"1.57.0",
             Go_build::VersionNumber=v"1.16.3",
-            archive_type::Symbol = (use_squashfs ? :squashfs : :unpacked),
+            archive_type::Symbol = (use_squashfs[] ? :squashfs : :unpacked),
             bootstrap_list::Vector{Symbol} = bootstrap_list,
             # Because GCC has lots of compatibility issues, we always default to
             # the earliest version possible.
@@ -714,7 +714,7 @@ matching.  If the given `Platform` already specifies a `libgfortran_version`
 """
 function expand_gfortran_versions(platform::AbstractPlatform)
     # If this platform is already explicitly libgfortran-versioned, exit out fast here.
-    if libgfortran_version(platform) != nothing
+    if libgfortran_version(platform) !== nothing
         return [platform]
     end
 
@@ -857,11 +857,11 @@ function preferred_libgfortran_version(platform::AbstractPlatform, shard::Compil
     if shard.name != "GCCBootstrap"
         error("Shard must be `GCCBootstrap`")
     end
-    if arch(shard.target) != arch(platform) || libc(shard.target) != libc(platform)
+    if arch(shard.target::Platform) != arch(platform) || libc(shard.target::Platform) != libc(platform)
         error("Incompatible platform and shard target")
     end
 
-    if libgfortran_version(platform) != nothing
+    if libgfortran_version(platform) !== nothing
         # Here we can't use `shard.target` because the shard always has the
         # target as ABI-agnostic, thus we have also to ask for the platform.
         return libgfortran_version(platform)
@@ -887,11 +887,11 @@ function preferred_cxxstring_abi(platform::AbstractPlatform, shard::CompilerShar
     if shard.name != "GCCBootstrap"
         error("Shard must be `GCCBootstrap`")
     end
-    if arch(shard.target) != arch(platform) || libc(shard.target) != libc(platform)
+    if arch(shard.target::Platform) != arch(platform) || libc(shard.target::Platform) != libc(platform)
         error("Incompatible platform and shard target")
     end
 
-    if cxxstring_abi(platform) != nothing
+    if cxxstring_abi(platform) !== nothing
         # Here we can't use `shard.target` because the shard always has the
         # target as ABI-agnostic, thus we have also to ask for the platform.
         return cxxstring_abi(platform)
@@ -922,26 +922,26 @@ function download_all_artifacts(; verbose::Bool = false)
     )
 end
 
-_sudo_cmd = nothing
-function sudo_cmd()
+const _sudo_cmd = Ref{Union{Cmd,Nothing}}(nothing)
+function sudo_cmd()::Cmd
     global _sudo_cmd
 
     # Use cached value if we've already run this
-    if _sudo_cmd != nothing
-        return _sudo_cmd
+    if _sudo_cmd[] !== nothing
+        return _sudo_cmd[]::Cmd
     end
 
-    if getuid() == 0
+    _sudo_cmd[] = if getuid() == 0
         # If we're already root, don't use any kind of sudo program
-        _sudo_cmd = ``
+        ``
     elseif success(`sudo -V`)
         # If `sudo` is available, use that
-        _sudo_cmd = `sudo`
+        `sudo`
     else
         # Fall back to `su` if all else fails
-        _sudo_cmd = `su root -c`
+        `su root -c`
     end
-    return _sudo_cmd
+    return _sudo_cmd[]::Cmd
 end
 
 """