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 to avoid invalidations, keep types #3742

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Dec 22, 2023

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

mkitti commented Dec 22, 2023

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

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.

Yes, this is my favorite attempt so far!

@mkitti
Copy link
Contributor Author

mkitti commented Dec 24, 2023

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 mkitti force-pushed the mkitti-refactor-binaryplatforms-compat-with-types branch from 103f2c5 to 4064a14 Compare April 13, 2024 22:41
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.

Pkg.BinaryPlatforms invalidates Base.BinaryPlatforms
2 participants