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

Revert "Extensions: make loading of extensions independent of what packages are in the sysimage (#52841) #56234

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

KristofferC
Copy link
Member

This reverts commit 08d229f.

There are some bugs now where extensions do not load when their package has been put into the sysimage. #52841 was made because it was common to get cycles otherwise but with #55589 that should be much less of a problem.

Subsumes #54750.

@KristofferC KristofferC added backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Oct 18, 2024
Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - nice to lift this restriction (which afaiu wasn't really solving our cycle problems anyway)

I didn't look much at the code since I'm assuming this is a clean revert, but the revert seems like the right thing to do

@@ -974,14 +974,14 @@ function explicit_manifest_deps_get(project_file::String, where::PkgId, name::St
entry = entry::Dict{String, Any}
uuid = get(entry, "uuid", nothing)::Union{String, Nothing}
uuid === nothing && continue
# deps is either a list of names (deps = ["DepA", "DepB"]) or
# a table of entries (deps = {"DepA" = "6ea...", "DepB" = "55d..."}
deps = get(entry, "deps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this deps lookup fix related? Does it need a test to match?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is partly related since this runs into #55939 due to how the manifest change with the revert. So it does in some sense already has a test.

@KristofferC KristofferC merged commit 5b677a1 into master Oct 22, 2024
7 checks passed
@KristofferC KristofferC deleted the kc/revert_sysimnage_ext branch October 22, 2024 11:44
topolarity added a commit that referenced this pull request Nov 22, 2024
…ns independent of what packages are in the sysimage (#52841)" (#56658)

This is a backport of #56234

It reverts commit 08d229f (and
backports ad1dc39, which was included
in the revert PR).
@topolarity topolarity removed the backport 1.11 Change should be backported to release-1.11 label Nov 23, 2024
@topolarity
Copy link
Member

topolarity commented Nov 23, 2024

@KristofferC We don't need any of this on 1.10, right?

I believe we merged an equivalent to ad1dc39 at some point, but wanted to double check

edit: Ah yep, that was 385caed - I'm going to remove the backport label since I think we got everything here.

@topolarity topolarity removed the backport 1.10 Change should be backported to the 1.10 release label Nov 23, 2024
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.

2 participants