Fix Pkg.BinaryPlatforms invalidations by moving module to Base

[mkitti]

• Move Pkg.BinaryPlatforms compatibility shim to Base.BinaryPlatforms.PkgCompat
• Move UnstableIO from Pkg.jl to Base

Address Pkg.jl invalidations by moving code to Base
JuliaLang/Pkg.jl#3702

Edits

• The UnstableIO portion was removed in favor of using IOContext{IO} instead: Replace UnstableIO with IOContext{IO} Pkg.jl#3735
• Background reading for how Base.BinaryPlatforms came to Base and its relationship with Pkg.BinaryPlatforms: Move Artifacts and BinaryPlatforms into Base #37320

[Keno]

This is probably beside the point, but this is insufficient to avoid inference inferring these functions on specialized IO. For that you'd need to add an extra inferencebarrier in the UnstableIO constructor.

[KristofferC]

It worked pretty well in practice. :p

[mkitti]

@Keno, would that look like the following?

struct UnstableIO <: IO
    io::IO
    UnstableIO(@nospecialize(x)) = new(inferencebarrier(x))
end

[mkitti]

@Keno I added an inferencebarrier in 1f0876f

[vtjnash]

This struct also doesn't seem compliant with the IO interface, so I am very reluctant to have this merged into Base. Perhaps make this <: AbstractPipe might suffice though, as then you get all of the required methods "for free"

[mkitti]

There's an IO interface? Let me see if I can find it. I'm going to see if I can figure this out quickly. Otherwise, I will separate this into another pull request.

[staticfloat]

LGTM

[ararslan]

Were there no tests in Pkg for these that could be copied over as well?

[mkitti]

I can port over https://github.com/JuliaLang/Pkg.jl/blob/master/test/binaryplatforms.jl

[mkitti]

I ported over the BinaryPlatforms tests in
9843555 .

I added Base.UnstableIO tests in 172909b

[mkitti]

@vtjnash
I refactored UnstableIO onto AbstractPipe in b55dc3d

[mkitti]

Is there anything else to do here? Should I split the PR into two parts?

[vtjnash]

Would it be easier to make Pkg instead use an IOContext{IO}(io), since it has the same implementation as this, without needing to add code to Base just for Pkg?

[mkitti]

I was thinking about that, but IOContext has a type parameter indicating the parent type. I thought the objective was to create an abstract inference barrier so as to not force recompilation of Pkg methods for new IO types.

I'm thinking of splitting the pull request so that we can focus on this issue.

[vtjnash]

That is true of the usual interface, but it is valid for the parameter to be simply IO also. Someone could make a PR to make that constructor a bit simpler also, as right now it is potentially a bit awkwardly specified as IOContext{IO}(unwrapcontext(io)...) and does not reliably propagate when making a new IOContext from an existing one with more keys.

[mkitti]

I see so I may be able to define the following.

UnstableIO(io:IO) = IOContext{IO}(inferencebarrier(io))

[mkitti]

Unless I hear otherwise, I intend to drop the UnstableIO part of the PR from this pull request.

[vtjnash]

Why do we need all the other stuff here too? It seems like if Pkg wants to export something with a similar name that does something different it should export something different, rather than pirating the method from Base or expecting Base to contain that functionality.

[mkitti]

Right now I'm looking for pragmatic steps to take to improve load times of Pkg as an independent package. My overarching motivation is to reduce the negative impact on the user experience that breaking these packages out creates.

To be fair, it was Base that committed the piracy against Pkg. This was originally a Pkg API.

The code here is a compatability shim because the API changed when this functionality moved from Pkg to Base. Now that Pkg.jl is no longer part of the system image, we have invalidation issues due that prior solution. While one could debate this, we are obligated to carry that compatibility shim, I believe.

I'm going to invite @staticfloat and @giordano to comment on where the older BinaryPlatforms API is still used and why we need the compatibility shim.

[mkitti]

I proposed an alternative approach is JuliaLang/Pkg.jl#3736 . That change is breaking in that it eliminates the ability to dispatch on the Linux, Windows, and MacOS types. Those are now constructors for Base.BinaryPlatforms.Platform. Surveying JuliaHub for use of the symbols, only a single registered package, AppBundler.jl, was found to be affected by the breakage so far.

[ararslan]

FWIW the point of those types was to enable dispatch, which I've personally used extensively, just not in any registered packages.

[mkitti]

I have proposed another alternative in JuliaLang/Pkg.jl#3742 , this time keeping the types. To avoid invalidations, I do not subtype Base.BinaryPlatforms.AbstractPlatform.