Support augment_platform in JLLWrappers

[vchuravy]

@staticfloat any ideas for tests?

[staticfloat]

For tests, I say let's add some once we have a package that uses this functionality, and then just add it to the test suite, like we did for OpenLibm_jll and Vulkan_Headers_jll.

[vchuravy]

@staticfloat The problem is that Pkg just picks "a" matching artifact irrespective of the tags (https://github.com/JuliaLang/julia/blob/dcaf356ff64fd3dc854d7732d1f60174b60c2795/stdlib/Artifacts/src/Artifacts.jl#L392) artifact_meta is used by Pkg's ensure_installed and of course that is the unmodified HostPlatform.

julia> import Pkg

julia> artifact_dict = Pkg.Artifacts.load_artifacts_toml("test/LAMMPS_jll/Artifacts.toml")
Dict{String, Any} with 1 entry:
  "LAMMPS" => Any[Dict{String, Any}("libgfortran_version"=>"4.0.0", "arch"=>"aarch64", "git-tree-sha1"=>"fb75bd7f2053d9459b9232846544753b40b13af5", "mp…

julia> platform = Pkg.Artifacts.HostPlatform()
Linux x86_64 {cxxstring_abi=cxx11, julia_version=1.7.0, libc=glibc, libgfortran_version=5.0.0, libstdcxx_version=3.4.29}

julia> meta = Pkg.Artifacts.artifact_meta("LAMMPS", artifact_dict, "test/LAMMPS_jll/Artifacts.toml"; platform)
Dict{String, Any} with 8 entries:
  "libgfortran_version" => "5.0.0"
  "arch"                => "x86_64"
  "git-tree-sha1"       => "3e6992505412f66641707fa611138c36211abefb"
  "mpi"                 => "mpitrampoline"
  "libc"                => "glibc"
  "download"            => Any[Dict{String, Any}("sha256"=>"04388ac8604ed2b969a275f08ab153f82cd3f9e9b5499a4db281e720c49516eb", "url"=>"https://github.c…
  "cxxstring_abi"       => "cxx11"
  "os"                  => "linux"

Compare:

julia> using Base.BinaryPlatforms

julia> BinaryPlatforms.add_tag!(platform.tags, "mpi", string("MPICH"))
"mpich"

julia> platform
Linux x86_64 {cxxstring_abi=cxx11, julia_version=1.7.0, libc=glibc, libgfortran_version=5.0.0, libstdcxx_version=3.4.29, mpi=mpich}

julia> meta = Pkg.Artifacts.artifact_meta("LAMMPS", artifact_dict, "test/LAMMPS_jll/Artifacts.toml"; platform)
Dict{String, Any} with 8 entries:
  "libgfortran_version" => "5.0.0"
  "arch"                => "x86_64"
  "git-tree-sha1"       => "8e1900715a9a41b8222a9b6a27460de55124c030"
  "mpi"                 => "mpich"
  "libc"                => "glibc"
  "download"            => Any[Dict{String, Any}("sha256"=>"7700dd737bc97922b6b92bbd10b0d0fb3e0b3021a8c49ca93b65c31e98831d6f", "url"=>"https://github.c…
  "cxxstring_abi"       => "cxx11"
  "os"                  => "linux"

So CI fails since we don't instantiate the mpich variant. Of course Pkg has no idea about our platform tag.

[staticfloat]

Of course Pkg has no idea about our platform tag.

That's the whole reason we have the Pkg hooks, so that we can tell Pkg how to add platform tags to its internal operations, like it will within collect_artifacts(). That collect_artifacts() should be what is used by instantiate(), so as long as those files exist, they should be augmenting the platform objects on-the-fly for all Pkg operations.

Can we scatter some debugging stuff to see precisely what platforms are being passed in? Try printing things out to stderr in the hooks to see if we can ensure they're actually being invoked.

Many changes afterwards

[vchuravy]

The Drone CI failure is weird. I get the Alpine one since I didn't build LAMMPS for it, but aarch64 should be fine.

[giordano]

Unless it's libgfortran3 😛 I don't think that's the case, but maybe double check by printing the host platform?

[vchuravy]

I accidentally deleted one of the wrapper files xD. Exactly the one tested there.

[vchuravy]

On Julia 1.6.3

Without 0.062860 seconds (272.55 k allocations: 16.151 MiB, 15.16% compilation time)
With 0.073066 seconds (320.50 k allocations: 20.016 MiB, 7.09% gc time, 9.72% compilation time)

On GTK3_jll

[vchuravy]

The majority of the allocations is stemming from eab77c9 causing us to fall into the non-static branch in https://github.com/JuliaLang/julia/blob/7be0dcdacdac28da56b9d607b1bc5f7b23ecb0e9/stdlib/Artifacts/src/Artifacts.jl#L674

[vchuravy]

After optimization with @giordano we are at:

Without (current master): 0.068104 seconds (288.67 k allocations: 17.615 MiB, 12.63% compilation time)
With: 0.068776 seconds (293.39 k allocations: 17.945 MiB, 10.28% compilation time)

[staticfloat]

That's pretty good; but I am still a little uncomfortable with us adding in extra dynamism to something that (usually) shouldn't have it. I had a big argument with Jameson about this, but erroring out when an artifact is not found (when the artifact is purportedly already installed) is a feature, not a bug. Part of it is to protect against the very situations that you bring up, e.g. running on a cluster and having 100 different nodes all try to install to the same depot at once.

For this reason, there was a module-level separation of functionality established; using Artifacts declares that you expect everything to exist, and if it doesn't exist, the behavior is to error out. using LazyArtifacts declares that you're okay with dynamically loading Pkg and fetching things from the network if things fail.

Perhaps we can have our cake and eat it too; the .pkg/select_artifacts.jl functionality gives us an awful lot of freedom. We can, for instance, pass include_lazy=true to select_downloadable_artifacts() to include all lazy artifacts to get them installed ahead-of-time, according to the platform that was found at Pkg.add() time.

As long as we use LazyArtifacts (or the lazy loading of Pkg that this PR includes, which this PR kind of duplicates from LazyArtifacts) we still run the risk of having a different platform on the worker nodes slam the filesystem though. The only way around that is to exclusively use Artifacts and turn platform mismatches into errors.

[vchuravy]

Perhaps we can have our cake and eat it too; the .pkg/select_artifacts.jl functionality gives us an awful lot of freedom. We can, for instance, pass include_lazy=true to select_downloadable_artifacts() to include all lazy artifacts to get them installed ahead-of-time, according to the platform that was found at Pkg.add() time.

That is an excellent idea. This would mitigate it fully for 1.7, only problem is that despite what the docs in PR says the support for .pkg/select_artifacts.jl is not available on 1.6. I just tried to do this, but it looks like select_downloadable_artifacts will only allow one artifact per name. Trying to fix this was part of the motivation behind JuliaLang/julia#43484 I just spent a bit poking at this, and I got to the point where I needed to implement a filter artifact by platform function that ignored the tag (if the tag is empty none of the artifacts match).

I agree that the dynamism is a major wart. I am less dogmatic about it since we already allow people to do many silly things.
I don't see the qualitative difference between opt-in laziness and pragmatic laziness to enable a sensible UX. I would be okay to only enable the laziness on 1.6 since otherwise it will just not work there.

Stale

[giordano]

This looks good to me, especially on the performance side. I did some more testing loading GTK3_jll, and its performance didn't change noticeably compared to master. Memory usage slightly increased (less than 1%, as already shown by #35 (comment)), but that should be fine.

I guess you need to address Elliot's concerns though 🙂

[vchuravy]

This should be okay right? Otherwise we can't load TOML in all circumstances.

[staticfloat]

Hmmm, in what circumstances does this not work? I didn't need to do this for https://github.com/JuliaLang/Pkg.jl/blob/master/test/test_packages/AugmentedPlatform/.pkg/select_artifacts.jl

[giordano]

I thought TOML was in Base? https://github.com/JuliaLang/julia/blob/b9432a8067a5bff7c1286c8e64b0572d5bd7a22c/base/toml_parser.jl#L3

[staticfloat]

That's only a parser; if you want to write TOML output (like we do here) you need to load the stdlib.

[vchuravy]

Funnily enough the testsuite here.

If I comment this out (on 1.7.1):

   Resolving package versions...
ERROR: LoadError: ArgumentError: Package TOML not found in current path:
- Run `import Pkg; Pkg.add("TOML")` to install the TOML package.

Stacktrace:
 [1] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:967
 [2] include(fname::String)
   @ Base.MainInclude ./client.jl:451
 [3] top-level scope
   @ none:5
in expression starting at /home/vchuravy/src/JLLWrappers/test/LAMMPS_jll/.pkg/select_artifacts.jl:8
  Stacktrace:
    [1] pipeline_error
      @ ./process.jl:531 [inlined]
    [2] read(cmd::Cmd)
      @ Base ./process.jl:418
    [3] collect_artifacts(pkg_root::String; platform::Base.BinaryPlatforms.Platform)
      @ Pkg.Operations /usr/share/julia/stdlib/v1.7/Pkg/src/Operations.jl:587
    [4] download_artifacts(env::Pkg.Types.EnvCache; platform::Base.BinaryPlatforms.Platform, julia_version::VersionNumber, verbose::Bool, io::Base.TTY)
      @ Pkg.Operations /usr/share/julia/stdlib/v1.7/Pkg/src/Operations.jl:614
    [5] develop(ctx::Pkg.Types.Context, pkgs::Vector{Pkg.Types.PackageSpec}, new_git::Set{Base.UUID}; preserve::Pkg.Types.PreserveLevel, platform::Base.BinaryPlatforms.Platform)

Adding a println(stderr, Base.LOAD_PATH) shows that the LOAD_PATH at that time is:

["@", "/tmp/jl_NEdS9m"]

which is also the same at the time of the Pkg.develop(PackageSpec(path=joinpath(@__DIR__, "LAMMPS_jll"))) call.

[staticfloat]

An alternative to pushing @StdLib to the LOAD_PATH is to add TOML to the LAMMPS_jll dependencies and then
push!(Base.LOAD_PATH, dirname(@DIR)).

Yep, that's what Kristoffer and I were talking about, and what I was referring to when I said It's not too onerous to require all these packages to declare a dependency on TOML though. I think I could get behind that.

But while working on JuliaLang/Pkg.jl#2920 we run into issues were loading Preferences would fail in a fresh depot.

You shouldn't need to load Preferences; you should be able to just use Base.get_preferences().

[vchuravy]

Right, hypothetical world where we move TOML out of the stdlib we might encounter a similar problem.

Are we in agreement then, that we should add TOML as a dependency, and then push!(Base.LOAD_PATH, dirname(@__DIR__))?

[staticfloat]

Why does JuliaLang/Pkg.jl@9e770ea not suffice for loading TOML if it is listed as a dependency?

[vchuravy]

Because we are setting --project=$(Base.active_project()). Which is not the same as --project=LAMMPS_jll.

We want to inherit preferences from the current user environment, but that does not need to have TOML declared.

[staticfloat]

Right, right of course. Okay yeah. I'm happy with that push!(Base.LOAD_PATH, dirname(@__DIR__)).