-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Remove incomplete manifest package load fallback. Add instantiate suggestion to error #42822
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
Remove incomplete manifest package load fallback. Add instantiate suggestion to error #42822
Conversation
3d71dc8 to
27375d1
Compare
27375d1 to
139e507
Compare
|
FWIW, #27932 was introduced at a time where projects were completely new and people were quite frustrated with not being able to load their package. So I would say it was a necessary "smoothener" in the transition to project-centered package loading but by now it might be a good idea to just remove that fallback. |
|
That sounds good actually. I'll turn this PR into that |
139e507 to
b6005c1
Compare
|
Now reverts #27932 but keeps the new first bullet in the suggestion |
|
@KristofferC What do you think? Give it a go for 1.8? |
|
If now's not the time for this, I don't think it needs to stay open. Easy to reopen |
|
I think this was essentially approved (and closes #42873). Sorry it got forgotten and nobody remembered to comment that it was good to merge. This code (that this removes) definitely injects a lot of bugs into the child processes, and only ever was possibly functional for the main process. |
|
@KristofferC just checking you agree? |
|
Sure, let's try this. |
|
An example of what this breaks cc. @timholy |
|
For the record, that breakage is a test of "does Revise behave the same as Base?" If Base is changing, Revise should too, and in this case I'd just not run that test on |
When packages are not instantiated at all, but present in the active environment, you get a helpful
but in some condition where some but not all packages are instantiated in some form you get a verbose very long mess of warnings.
Seemingly the tooling intended for dev juggling in #27932 is being hit in non-dev situations
as seen in https://discourse.julialang.org/t/the-endless-packages-to-add-as-as-a-dependency-or-to-resolve/70450
The loading code does already intend to make warnings manageable by suppressing repeat warnings, but the flags aren't sent to the child
compilecacheworkers, so they think they've not been seen before.I once managed to replicate the issue above by checking out the repo in question, activating it and
using Imageswithout instantiating first, hit the error, then fixed it with a single instantiate.This PR
Pkg.instantiate()should be tried first.(a bugfix) passes the existing warning state helpers to the nestedcompilecacheworkers, to make the suppression of repeat warnings work through code loadingIdeally it'd be good to figure out why #27932 gets hit and avoid that.I don't have a reproducer to test this out
Related to #38545