Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Pkg.BinaryPlatforms compat, fix invalidations #3736

Closed

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Dec 17, 2023

In JuliaLang/julia#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
Copy link
Contributor Author

mkitti commented Dec 17, 2023

@staticfloat , I would appreciate your review here.

Copy link
Sponsor Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing I'm worried about with this is that old code that depended on this will break. I'm thinking old BinaryBuilder versions, old BinaryProvider versions, etc... Of course none of that code is expected to be widespread, but this is technically breaking.

@mkitti
Copy link
Contributor Author

mkitti commented Dec 18, 2023

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 JuliaLang/julia#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
Copy link
Contributor Author

mkitti commented Dec 18, 2023

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
Copy link
Contributor Author

mkitti commented Dec 18, 2023

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
Copy link
Contributor Author

mkitti commented Dec 18, 2023

I found an example where this would break a relatively new package:
PeaceFounder/AppBundler.jl#4

@mkitti
Copy link
Contributor Author

mkitti commented Dec 18, 2023

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.

Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of defining these as types was to enable dispatch on platform, which I've personally used extensively. I do not think they should be removed. It's also used by VersionsJSONUtil which builds the versions.json file used by CI, juliaup(?), the website downloads page, et al.

@staticfloat
Copy link
Sponsor Member

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
Copy link
Contributor Author

mkitti commented Dec 22, 2023

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
Copy link
Contributor Author

mkitti commented Dec 22, 2023

I tried again, keeping the types this time:
#3742

I will close this in favor of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants