-
Notifications
You must be signed in to change notification settings - Fork 36
use julia_version Pkg.add kwarg rather than specifying it through ctx
#416
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
Conversation
e761246 to
5e8fb67
Compare
d5251de to
3375fca
Compare
|
@giordano this is good from my perspective now |
|
Haven't looked at the code yet, but this is promising, thanks! I guess a big test is to use this PR in JuliaPackaging/Yggdrasil#10524 (I probably won't have much time this week though). |
e175b6f to
10672f1
Compare
e13f11c to
713696c
Compare
| function filter_redundant_version(p::PkgSpec) | ||
| if p.version !== nothing && p.tree_hash !== nothing | ||
| if p.version != PKG_VERSIONS.VersionSpec("*") && p.tree_hash !== nothing | ||
| return Pkg.Types.PackageSpec(;name=p.name, tree_hash=p.tree_hash, repo=p.repo) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was wrong, it's not nothing when unset.
julia> pspec = Pkg.PackageSpec(
name = "LLVM_full_jll",
uuid = Base.UUID("a3ccf953-465e-511d-b87f-60a6490c289d"),
tree_hash = Pkg.Types.SHA1("14aa3ea8eb237d341af4f4571d91d17453d88d41"),
url = "https://github.com/JuliaBinaryWrappers/LLVM_full_jll.jl.git",
rev = "b0a36286a6a0e6f70324ceec1958a2bb494acdb4"
)
PackageSpec(
name = LLVM_full_jll
uuid = a3ccf953-465e-511d-b87f-60a6490c289d
tree_hash = 14aa3ea8eb237d341af4f4571d91d17453d88d41
url = https://github.com/JuliaBinaryWrappers/LLVM_full_jll.jl.git
rev = b0a36286a6a0e6f70324ceec1958a2bb494acdb4
version = *
)
julia> pspec.version
VersionSpec("*")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a regression test mentioning BBB in Pkg JuliaLang/Pkg.jl#4332
|
This is ready to go from my perspective |
giordano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks overall great, but the zstd path seems wrong.
src/Prefix.jl
Outdated
| # Zstd_jll became a stdlib in Julia v1.13.0 and the path variable changed name | ||
| zpath = isdefined(Zstd_jll, :libzstd_path) ? Zstd_jll.libzstd_path : Zstd_jll.zstd_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sigh. But also, this doesn't look right: the library and the executable are in two different directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For others following along: JuliaLang/julia#59198
Just need to wait for nightly to be 1a1270d977ba8aab7fcefe0d652f493bcc3dc44e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
See JuliaLang/Pkg.jl#4161 JuliaLang/Pkg.jl#4160
Tested on yggdrasil here: