PeaceFounder / AppBundler.jl

Bundle your Julia application
MIT License
53 stars 3 forks source link

Discouraging use of Pkg.BinaryPlatforms #4

Closed mkitti closed 7 months ago

mkitti commented 9 months ago

Over in Pkg.jl, I'm trying to fix invalidations. AppBunder is using the old Pkg.BinaryPlatforms API rather than sticking with the newer Base.BinaryPlatforms.

One of my proposed fixes involves making Pkg.BinaryPlatforms.Linux and the related platform types into a constructor for Base.BinaryPlatforms.Platform. Methods such as Sys.islinux should continue to work in the expected way.

julia> Pkg.BinaryPlatforms.Linux(:x86_64) |> typeof
Base.BinaryPlatforms.Platform

julia> Sys.islinux(Pkg.BinaryPlatforms.Linux(:x86_64))
true

Could you consider refactoring to not use Pkg.BinaryPlatforms.Linux as a type?

https://github.com/JuliaLang/Pkg.jl/pull/3736

JanisErdmanis commented 9 months ago

I am unsure if I would like to go the Val route. The function signatures would get longer with additional type information instead of being picked up directly from a platform type.

An alternative route would be to subtype AbstractPlatform in AppBundler to create Linux, MacOS, and Windows types. I have to think about this more.

mkitti commented 9 months ago

Using Val to dispatch in this way is roughly equivalent to appending a _linux, _windows, and _macos version of the function. Val here is used just to dispatch the primary function.

The main reason for abandoning the Linux <: AbstractPlatform approach is because this leads to excess specialization and thus additional compilation and latency.

With Linux <: AbstractPlatform you would need a specialized arch or unwrap and thus get many versions of these functions for each operating system. You could define an unwrap with @nospecialize, but now you end up doing dynamic dispatch.

The other reason for abandoning Linux <: AbstractPlatform is this invalidates a lot of code in the system image leading to recompilation. This problem arises now since Pkg is no longer part of the system image as of Julia 1.11.

Anyways, I'm just letting you know that Pkg.BinaryPlatform is a deprecated, undocumented, and non-public API. You are the first one to try to use it in three years.

JanisErdmanis commented 7 months ago

Closing as this change is no longer needed, according to https://github.com/JuliaLang/Pkg.jl/pull/3742

mkitti commented 7 months ago

Note that https://github.com/JuliaLang/Pkg.jl/pull/3742 introduces a new problem at least temporarily.

Types in Pkg.BinaryPlatforms would no longer be subtypes of Base.BinaryPlatforms.AbstractPlatform, at least temporarily.

Subsequent reorganization of Base.BinaryPlatforms might be able to restore the relationship.

JanisErdmanis commented 7 months ago

That is good to know. That seems, however, a minor convenience as I will simply define const AbstractPlatform = Union{Linux, MacOS, Windows, Platform} and leave everything else as is.

mkitti commented 7 months ago

That PR creates a Pkg.BinaryPlatforms.AbstractPlatform. You might want

const AbstractPlatforms = Union{Base.BinaryPlatforms.AbstractPlatform, Pkg.BinaryPlatforms.AbstractPlatform}

I think that works today in 1.10 and should collapse to Base.BinaryPlatforms.AbstractPlatform.

julia> using Pkg

julia> const AbstractPlatforms = Union{Base.BinaryPlatforms.AbstractPlatform, Pkg.BinaryPlatforms.AbstractPlatform}
Base.BinaryPlatforms.AbstractPlatform

Then again, nothing has been merged.