JuliaLang / Pkg.jl

Pkg - Package manager for the Julia programming language
https://pkgdocs.julialang.org
Other
616 stars 258 forks source link

Refactor Pkg.BinaryPlatforms compat, fix invalidations #3736

Closed mkitti closed 8 months ago

mkitti commented 8 months ago

In https://github.com/JuliaLang/julia/pull/52249 I proposed moving the Pkg.BinaryPlatforms to base in order to fix invalidations.

Here, I pursue an alternate route and refactor Pkg.BinaryPlatforms. In particular, I eliminate the AbstractPlatform subtypes. UnknownPlatform, Linux, Windows, MacOS, and FreeBSD are now just methods that return Base.BinaryPlatforms.Platform instances.

No methods are imported from Base.BinaryPlatforms and no methods are overloaded. arch, libc, call_ab, cxxstring_abi which used to be methods of Base.BinaryPlatforms are now just independent functions belong Pkg.BinaryPlatforms. The Pkg.BinaryPlatform versions return Symbol or Nothing. The Base.BinaryPlatform versions return String or Nothing.

The test/binaryplatforms.jl test suite passes without modification.

Demonstration:

julia> using Pkg.BinaryPlatforms

julia> Linux
Linux (generic function with 1 method)

julia> Linux(:x86_64)
Linux x86_64 {libc=glibc}

julia> platform = Linux(:x86_64)
Linux x86_64 {libc=glibc}

julia> typeof(platform)
Platform

julia> arch(platform)
:x86_64

julia> Base.BinaryPlatforms.arch(platform)
"x86_64"

julia> libc(platform)
:glibc

julia> Base.BinaryPlatforms.libc(platform)
"glibc"

julia> call_abi(platform)

julia> Base.BinaryPlatforms.call_abi(platform)

julia> cxxstring_abi(platform)

julia> Base.BinaryPlatforms.cxxstring_abi(platform)
mkitti commented 8 months ago

@staticfloat , I would appreciate your review here.

mkitti commented 8 months ago

I agree that this could be breaking. What I would like to figure out is specifically how so? The test suite does not seem to capture it.

If we do find this unretrievably breaking, then that provides cause to push https://github.com/JuliaLang/julia/pull/52249 further. It also provides a direction to improve the test suite.

The main break I could see is if the former constructors were actually expected to create a distinct type or not. It is not clear to me from the tests that this is clearly the case.

mkitti commented 8 months ago

I just tried Blosc v0.6.0 that uses BinaryProvider v0.5.10 and everything seems to working as expected. I'll try some older packages and JLLs and see if I can get this to break.

(testenv) pkg> st
Status `~/src/Pkg.jl/testenv/Project.toml`
⌃ [a74b3585] Blosc v0.6.0
  [44cfe95a] Pkg v1.11.0 `..`

(testenv) pkg> st -m
Status `~/src/Pkg.jl/testenv/Manifest.toml`
  [9e28174c] BinDeps v1.0.2
  [b99e7846] BinaryProvider v0.5.10
⌃ [a74b3585] Blosc v0.6.0
...

julia> using Blosc

julia> Blosc.compress(rand(0x0:0x1, 1024)) |> length
568
mkitti commented 8 months ago

Actually, I do not think BinaryProvider is a problem here. The last released version was v0.5.10 which did not contain any references to Pkg.BinaryBuilder: https://github.com/JuliaPackaging/BinaryProvider.jl/tree/v0.5.10

The master branch does contain a reference, but it was never released as far as I can tell.

Thus I think it is only the old BinaryBuilder JLLs that we need to check. That's promising since most of those are probably auto-generated and quite regular.

mkitti commented 8 months ago

I found an example where this would break a relatively new package: https://github.com/PeaceFounder/AppBundler.jl/issues/4

mkitti commented 8 months ago

Searching for symbols, AppBundler.jl is the only registered package I could find that actually tries to dispatch on the types.

https://juliahub.com/ui/Search?q=MacOS&type=symbols https://juliahub.com/ui/Search?q=Windows&type=symbols https://juliahub.com/ui/Search?q=Linux&type=symbols

I created a pull request to modify AppBundler.jl to not dispatch on the types.

staticfloat commented 8 months ago

I do not think they should be removed.

To be clear, Base.BinaryPlatforms removed it a long time ago, because it was found to cause a lot of unfortunate compile time. These compat adapters have been in place since then to allow legacy code to continue to use it, but I would encourage all users to move away from the per-OS types to the unified Platform type. Not only is splitting the type based on OS bad for compile time, it's also kind of an arbitrary choice when there are lots of other ways to split platforms (architecture, libc, libgfortran version, etc...) that are much better represented with the unified Platform object.

mkitti commented 8 months ago

I do have another idea in mind. There is not a strong reason why we need to extend either the Base.BinaryPlatforms types or APIs. We could have a separate type hierarchy which converts and forwards to the Base methods.

I will try that as another pull request.

mkitti commented 8 months ago

I tried again, keeping the types this time: https://github.com/JuliaLang/Pkg.jl/pull/3742

I will close this in favor of that.