-
-
Notifications
You must be signed in to change notification settings - Fork 271
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
Refactor Pkg.BinaryPlatforms compat, fix invalidations #3736
Conversation
@staticfloat , I would appreciate your review here. |
There was a problem hiding this 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.
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. |
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 |
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: 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. |
I found an example where this would break a relatively new package: |
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 I created a pull request to modify AppBundler.jl to not dispatch on the types. |
There was a problem hiding this 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.
To be clear, |
I do have another idea in mind. There is not a strong reason why we need to extend either the I will try that as another pull request. |
I tried again, keeping the types this time: I will close this in favor of that. |
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 theAbstractPlatform
subtypes.UnknownPlatform
,Linux
,Windows
,MacOS
, andFreeBSD
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 bemethods of
Base.BinaryPlatforms
are now just independent functions belongPkg.BinaryPlatforms
. ThePkg.BinaryPlatform
versions returnSymbol
or
Nothing
. TheBase.BinaryPlatform
versions returnString
orNothing
.The
test/binaryplatforms.jl
test suite passes without modification.Demonstration: