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

Move Artifacts and BinaryPlatforms out of Pkg #2001

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

staticfloat
Copy link
Member

This PR guts the Artifacts and BinaryPlatforms modules, replacing them with imports from Base and stdlibs. This will obviously not pass until JuliaLang/julia#37320 is merged.

@staticfloat staticfloat force-pushed the sf/they_grow_up_so_fast branch 2 times, most recently from a1eb91e to 0956a93 Compare September 10, 2020 09:24
@staticfloat
Copy link
Member Author

This will fail until we merge JuliaLang/julia#37320, but on my local machine, when run with that version it passes tests.

This splits `BinaryPlatforms` and `Artifacts` out of `Pkg` and into
`Base`/a separate stdlib.  In the process, we significantly reworked
`BinaryPlatforms`, and so to maintain backwards-compatibility, we
define a set of compatability functions.
@staticfloat staticfloat force-pushed the sf/they_grow_up_so_fast branch from 0956a93 to e567349 Compare September 14, 2020 22:28
@staticfloat staticfloat merged commit e6399b7 into master Sep 15, 2020
@staticfloat staticfloat deleted the sf/they_grow_up_so_fast branch September 15, 2020 17:13
@StefanKarpinski
Copy link
Member

YES!!

@timholy
Copy link
Member

timholy commented Sep 15, 2020

Even the name of this branch is awesome.

@timholy
Copy link
Member

timholy commented Mar 5, 2022

How much of BinaryPlatforms_compat.jl do we still need? It causes problems:

julia> methods(Base.BinaryPlatforms.libgfortran_version, (Base.BinaryPlatforms.AbstractPlatform,))
# 5 methods for generic function "libgfortran_version":
[1] libgfortran_version(p::Pkg.BinaryPlatforms.Linux) in Pkg.BinaryPlatforms at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.9/Pkg/src/BinaryPlatforms_compat.jl:80
[2] libgfortran_version(p::Pkg.BinaryPlatforms.Windows) in Pkg.BinaryPlatforms at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.9/Pkg/src/BinaryPlatforms_compat.jl:80
[3] libgfortran_version(p::Pkg.BinaryPlatforms.MacOS) in Pkg.BinaryPlatforms at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.9/Pkg/src/BinaryPlatforms_compat.jl:80
[4] libgfortran_version(p::Pkg.BinaryPlatforms.FreeBSD) in Pkg.BinaryPlatforms at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.9/Pkg/src/BinaryPlatforms_compat.jl:80
[5] libgfortran_version(p::Base.BinaryPlatforms.AbstractPlatform) in Base.BinaryPlatforms at binaryplatforms.jl:453

5 is more than the limit of 3 for successful abstract inference. It means that calls like Base.BinaryPlatforms.libgfortran_version get inferred as returning Any, even though all 5 methods return a Union{Nothing, VersionNumber}. This in turn leads to about 1800 invalidations from loading a package like InlineStrings.jl, including invalidation of Base.require which is always bad news.

Unless we delete these "compat" methods, every call site needs to be annotated ::Union{Nothing, VersionNumber}. But if we can delete the methods in

for f in (:libgfortran_version, :libstdcxx_version, :platform_name, :wordsize, :platform_dlext, :tags, :triplet)
@eval begin
$(f)(p::$(T)) = $(f)(p.p)
end
end
then inference will be successful.

@staticfloat
Copy link
Member Author

I think we can try deleting it, but we'll probably need to do a PkgEval run to see how many people are actually still using these.

A quick julia hub code search shows that nobody other than JLL packages are attempting to use Pkg.BinaryPlatforms, but that may not be 100% reliable, as for instance, BinaryProvider uses it but doesn't show up in that search. I'm pretty sure this change would break BinaryProvider, so it would be good to know who that would break.

Alternatively, we can just simplify this code to be less dispatch-heavy. Those methods you've noticed at the end are the same for all of these "compat" types, so we can just simply hoist the definition out of the loop and provide a union type as the argument:

const PlatformUnion = Union{Linux,MacOS,Windows,FreeBSD}
for f in (:libgfortran_version, :libstdcxx_version, :platform_name, :wordsize, :platform_dlext, :tags, :triplet)
    @eval begin
        $(f)(p::PlatformUnion) = $(f)(p.p)
    end
end

@giordano
Copy link
Contributor

giordano commented Mar 7, 2022

I'm pretty sure this change would break BinaryProvider, so it would be good to know who that would break.

We can cap compatibility of BinaryProvider in the registry. It's already quite broken in latest releases.

@timholy
Copy link
Member

timholy commented Mar 7, 2022

I made a failed run at this in #3017. I just don't know enough about the requirements to do this well.

@timholy
Copy link
Member

timholy commented Mar 9, 2022

Thanks for the suggestion @staticfloat, that seemed to work well (#3020).

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.

5 participants