search

JuliaLang / Pkg.jl

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

Refactor Pkg.BinaryPlatforms to avoid invalidations, keep types #3742

Closed mkitti closed 2 days ago

mkitti commented 6 months ago

This is my third attempt to address invalidations caused by Pkg.BinaryPlatforms.

The prior attempts were

In this pull request, I introduce a new Pkg.BinaryPlatforms.AbstractPlatform type that is distinct from Base.BinaryPlatforms.AbstractPlatform. The types here, Linux, Windows, MacOS, and FreeBSD subtype Pkg.BinaryPlatforms.AbstractPlatform.

Because of this, these types no longer invalidate any Base methods as detailed in

3702

Fixes #3702 (in part).

mkitti commented 6 months ago

After this pull request, the only remaining invalidations relate to UnstableIO.

julia> using SnoopCompileCore

julia> invalidations = @snoopr begin
           using Pkg
           Pkg.activate(".")
       end;
  Activating project at `~/src/Pkg.jl`

julia> using SnoopCompile
[ Info: Precompiling SnoopCompile [aa65fe97-06da-5843-b5b1-d5d13cad87d2]

julia> invalidation_trees(invalidations)
1-element Vector{SnoopCompile.MethodInvalidations}:
 inserting print(io::Pkg.UnstableIO, arg::Union{SubString{String}, String}) @ Pkg ~/src/Pkg.jl/src/Pkg.jl:49 invalidated:
   backedges: 1: superseding print(xs...) @ Base coreio.jl:3 with MethodInstance for print(::Any, ::String) (2 children)
              2: superseding print(io::IO, s::Union{SubString{String}, String}) @ Base strings/io.jl:250 with MethodInstance for print(::IO, ::String) (362 children)
   1 mt_cache
mkitti commented 6 months ago

The main breaking change is that Linux is no longer a subtype of Base.BinaryPlatforms.AbstractPlatform, henceforth referred to as BBP.AbstractPlatform.

BBP.AbstractPlatform was meant to ease the implementation of Pkg.BinaryPlatforms. Now BBP.AbstractPlatform has no purpose if this is merged. BBP.Platform would be its only subtype.

A complementary change on the Base side would be to define methods for BBP.Platform rather than BBP.AbstractPlatform. After doing so, BBP.AbstractPlatform may be free again to act as a common abstract parent type for BBP.Platform and the OS-specific types.

mkitti commented 6 days ago

The invalidation issue has been resolved.

IanButterworth commented 3 days ago

Given @staticfloat's approval, if you resolve the conflicts here I can merge it if green @mkitti

mkitti commented 2 days ago

This needs to reviewed again in light of BinaryBuilder2 and other fixes to invalidations.

In short, there are fewer uses of AbstractPlatform in the system image now. Particularly I changed a Dict{AbstractPlatform} to a Dict{Platform}.

mkitti commented 2 days ago

This needs to reviewed again in light of BinaryBuilder2 and other fixes to invalidations.

In short, there are fewer uses of AbstractPlatform in the system image now. Particularly I changed a Dict{AbstractPlatform} to a Dict{Platform}.