-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
Attempt to fix BB julia_version issue #2942 #2963
base: master
Are you sure you want to change the base?
Conversation
How do we get the dependencies for that stdlib then, to put in the manifest? |
Good point, I moved the check to the |
I don't know if the test failures here are my doing... |
I believe the MacOS failure is a flaky test. The other is fixed on master. You just need to rebase/update. Is there a test that could be added? |
Regarding tests: the one package this affects right now is |
Rebased and added test, which passes locally 🤞 |
@IanButterworth any chance to get this merged in a way that it gets into Julia 1.8? Or is that too late already? |
Sorry, I've tried to understand this, but I can't. I think the logic here might need comments in the code. It seems a reasonable thing to get into 1.8 IMO |
The change (line 431 in Operations.jl) checks if the dependency is still an stdlib in the current running version of Julia, since |
Sound like a great comment to add. Also, might "download unavailable stdlibs during julia_version resolve" be a descriptive name for this PR? |
@@ -428,7 +428,7 @@ function deps_graph(env::EnvCache, registries::Vector{Registry.RegistryInstance} | |||
# unregistered stdlib we must special-case it here. This is further | |||
# complicated by the fact that we can ask this question relative to | |||
# a Julia version. | |||
if is_unregistered_stdlib(uuid) || uuid_is_stdlib | |||
if (is_unregistered_stdlib(uuid) || uuid_is_stdlib) && haskey(stdlibs(), uuid) |
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.
I'm confused, as far as I can tell, this branch should already be false
:
julia> using Pkg
uuid = Base.UUID("a83860b7-747b-57cf-bf1f-3e79990d037f")
stdlibs_for_julia_version = Pkg.Types.get_last_stdlibs(v"1.6.0")
haskey(stdlibs_for_julia_version, uuid)
true
julia> stdlibs_for_julia_version = Pkg.Types.get_last_stdlibs(v"1.7.0")
haskey(stdlibs_for_julia_version, uuid)
false
julia> Pkg.Types.is_unregistered_stdlib(uuid)
false
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.
But the error occurs when doing:
Pkg.add(Pkg.Types.Context(; julia_version = v"1.6"), [PackageSpec(; name = "libjulia_jll")])
So uuid_is_stdlib
is true, and it tries to get the project file from an stdlib that is no longer there and crashes.
@@ -428,7 +428,7 @@ function deps_graph(env::EnvCache, registries::Vector{Registry.RegistryInstance} | |||
# unregistered stdlib we must special-case it here. This is further | |||
# complicated by the fact that we can ask this question relative to | |||
# a Julia version. | |||
if is_unregistered_stdlib(uuid) || uuid_is_stdlib | |||
if (is_unregistered_stdlib(uuid) || uuid_is_stdlib) && haskey(stdlibs(), uuid) |
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.
if (is_unregistered_stdlib(uuid) || uuid_is_stdlib) && haskey(stdlibs(), uuid) | |
if (is_unregistered_stdlib(uuid) || uuid_is_stdlib) && haskey(stdlibs(), uuid) | |
# if the dependency is still a stdlib in the current running version of Julia it doesn't need to be downloaded |
@barche does this seem correct?
Although, I don't understand how the correct version is installed for the target julia version if it's different to the one provided by the host Julia
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.
Yes, I think this is a correct summary. It's true that there could be edge cases where this gets an incompatible version of an stdlib, but only in the case where a package is requested for a julia version that is different from the running version. I don't know how to deal with that and I'd propose to tackle that problem when and if it ever occurs.
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.
@staticfloat it'd be good to get your take on this part too.
Although, I don't understand how the correct version is installed for the target julia version if it's different to the one provided by the host Julia
This basically skips an stdlib in
deps_graph
if it is no longer an stdlib in the current Julia version (and therefore doesn't exist). I'm not entirely sure I know what I'm doing :)