-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Move precompilation during package loading behind @invokelatest
#60727
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
Move precompilation during package loading behind @invokelatest
#60727
Conversation
|
(bump) |
|
@KristofferC, this seems like something you might want to take a look at. |
KristofferC
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.
Maybe add a comment
This should fix errors from IJulia and the VS Code extension that redirect stdio to a custom IO type. Packages are loaded in a fixed world-age to prevent invalidations but precompilation will print messages, so if stdout/stderr are set to types defined in a newer world than `Base._require_world_age` (almost guaranteed if from an external package) then `pipe_writer(custom_io)` will end up being called, which will error because it's running in a world before the methods for `custom_io` were defined.
560549c to
5dec715
Compare
|
Good point, added in 5dec715. |
|
(bump) |
|
@IanButterworth I took the liberty of removing the backport label for 1.10, I don't think it's necessary because #53403 was only merged in 1.11, 1.10 should still contain #51397. |
|
We backported precompilepkgs to 1.10. Can you review the 1.10 code to check. |
|
Hmm it doesn't look it was backported, the code is still calling the Pkg hook: Line 2033 in 95f30e5
It looks ok to me, unless the plan is to backport precompilepkgs now (in which case yeah this PR should also be backported). |
|
I can't find any evidence of it being backported.. odd. Maybe we tried and abandoned it or something. |
vtjnash
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.
This feels somewhat aggressive to possibly recompile this huge function in order just to do the IO only in a different world. But otherwise does LGTM
|
I can test it for invalidations later in the week. But last I looked at it'd be tricky to be more fine-grained because there's a lot of printing stuff in there. |
|
Will this be backported to 1.12? |
|
I think it will since it still has the 1.12 label. Probably a bit late for 1.12.5 though. |
This should fix errors from IJulia and the VS Code extension that redirect stdio to a custom IO type. Packages are loaded in a fixed world-age to prevent invalidations but precompilation will print messages, so if stdout/stderr are set to types defined in a newer world than
Base._require_world_age(almost guaranteed if from an external package) thenpipe_writer(custom_io)will end up being called, which will error because it's running in a world before the methods forcustom_iowere defined.This essentially restores #51397, which was removed in #53403.
Fixes #60223.