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

precompilepkgs: make the circular dep warning clearer and more informative #56621

Merged
merged 13 commits into from
Nov 22, 2024

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Nov 20, 2024

Was e.g.

┌ Warning: Circular dependency detected. Precompilation will be skipped for:
│   Base.PkgId(Base.UUID("eb0c05c4-6780-5852-a67e-5d31d2970b9a"), "ArrayInterfaceTrackerExt")
│   Base.PkgId(Base.UUID("f517fe37-dbe3-4b94-8317-1923a5111588"), "Polyester")
│   Base.PkgId(Base.UUID("0d7ed370-da01-4f52-bd93-41d350b8b718"), "StaticArrayInterface")
│   Base.PkgId(Base.UUID("6a4ca0a5-0e36-4168-a932-d9be78d558f1"), "AcceleratedKernels")
│   Base.PkgId(Base.UUID("244f68ed-b92b-5712-87ae-6c617c41e16a"), "NNlibAMDGPUExt")
│   Base.PkgId(Base.UUID("06b0261c-7a9b-5753-9bdf-fd6840237b4a"), "StaticArrayInterfaceStaticArraysExt")
│   Base.PkgId(Base.UUID("21141c5a-9bdb-4563-92ae-f87d6854732e"), "AMDGPU")
│   Base.PkgId(Base.UUID("9f7883ad-71c0-57eb-9f7f-b5c9e6d3789c"), "Tracker")
└ @ Base.Precompilation precompilation.jl:511

Now
Screenshot 2024-11-21 at 11 20 50 PM

Thanks to @topolarity figuring out proper cycles tracking.

@IanButterworth IanButterworth added the backport 1.11 Change should be backported to release-1.11 label Nov 20, 2024
@topolarity

This comment was marked as resolved.

@IanButterworth

This comment was marked as resolved.

@topolarity
Copy link
Member

We should probably backport to 1.10 too, since we expect that users will be hitting extension cycles a lot more often on that release.

In 1.11 these cycles are supposed to be impossible, except for environment tearing (where you end up with a package graph that doesn't actually ] resolve) and any implementation bugs (some of which were not fixed until.. yesterday 😉)

Still very good to have (esp. on 1.10), just want to be clear about whether this is a "normal" error any more

@topolarity topolarity added the backport 1.10 Change should be backported to the 1.10 release label Nov 21, 2024
base/precompilation.jl Outdated Show resolved Hide resolved
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.

Looking great! I think it's much clearer to users what's going on here now.

Thanks for iterating on this @IanButterworth

base/precompilation.jl Outdated Show resolved Hide resolved
base/precompilation.jl Outdated Show resolved Hide resolved
base/precompilation.jl Outdated Show resolved Hide resolved
@topolarity
Copy link
Member

Oh the cycle order does look a bit "backwards" to me

I would have expected the dependency of a package to be above it, not below

@IanButterworth
Copy link
Member Author

I would have expected the dependency of a package to be above it, not below

I think of it the other way. Matching the sentence order for "Foo depends on Bar, which depends on Baz"

Foo
 Bar
  Baz

@topolarity
Copy link
Member

Hmm.. What if we reverse the indentation from greatest to smallest?

  Baz
 Bar
Foo

Does that align our intuitions?

@topolarity topolarity changed the title precompilepkgs: make the circular dep report prettier precompilepkgs: make the circular dep warning clearer and more informative Nov 22, 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.

I still find the staircase weird here (and my first instinct is to read it "backwards"), but this is a big improvement and all the functionality looks ship-shape to me

Good to merge, I think!

@IanButterworth
Copy link
Member Author

Yeah, lets see what people make of it and tweak if there's concensus

@topolarity topolarity merged commit 0bedaae into JuliaLang:master Nov 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants