-
Notifications
You must be signed in to change notification settings - Fork 36
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
Should it be necessary to clear all
before unloading a pkg?
#1160
Comments
Do you mean |
I can reproduce this error locally. In fact, the minimal reproducible example for me is
For reference,
|
Could the underlying issue be Running
|
Running
So the reason that |
Makes sense. I guess I wonder what is correct here? Like if you have objects created from a package and then you unload that package, it seems to me you are asking for trouble... |
Is it possible to customize what My intuition is that More generally, it appears to me that sensible implementation of (
But of course, we may not be able to achieve this if it is not possible to customize what |
I don't recall there being a 'onUnload' hook, but I don't know. Take a look through the official package docs and then post a thread on discourse? The current onCleanup hook is supposed to stop us from losing the two file descriptors (of Popen2). |
Both the documentation
and the source code https://hg.savannah.gnu.org/hgweb/octave/file/04120d65778a/scripts/pkg/private/unload_packages.m don't indicate such functionality, I am pretty confident it doesn't exist. If all the current |
I see what you mean. I just looked at Seems to me this is a least somewhat an upstream issue? For reference: https://octave.sourceforge.io/octave/function/onCleanup.html How urgent do you think this is? I think I just noticed its a warning not an error so I think this is "paper cut" rather than bug... (?) |
So "needs decision" for question: "can we defer worrying about this?" If people are anything like me, they never "unload" packages in real life. And for the record, the MWE that users could reasonably expect is:
That is, without that |
I can't recall the last time I use Regarding "using some tricks", what I mean is that we can define conditional as an anonymous function and for other things such as sequential execution / unordered execution as well. The end result is that we can do almost everything w/o using any statement. For instance, the following example cleanup function runs correctly w/o any warning:
I don't think this will cause more maintenance burden because we don't have to change it once it's written. But we need comments to explain what's going on. |
wow! Shall we release as-is and see what happens? If you take your approach, maybe we can move (well "move back") all the
(EXCEPT that will be an error if they are already closed) |
Oh, how about? |
Yes, simplifying the cleanup function sounds like a good idea to me! |
|
This warning --8<---------------cut here---------------start------------->8--- warning: onCleanup: error caught while executing cleanup function: 'python_ipc_popen2_reset' undefined near line 78, column 30 "'python_ipc_popen2_reset' undefined" --8<---------------cut here---------------end--------------->8--- is caused by "pkg unload symbolic". We fix it by closing file descriptors and waitpid directly in the cleanup function instead of using private/python_ipc_popen2_reset. Fixes gnu-octave#1160. * inst/private/python_ipc_popen2.m: Close file descriptors and waitpid directly in the cleanup function. * inst/private/python_ipc_popen2_reset.m: Remove file.
This warning --8<---------------cut here---------------start------------->8--- warning: onCleanup: error caught while executing cleanup function: 'python_ipc_popen2_reset' undefined near line 78, column 30 "'python_ipc_popen2_reset' undefined" --8<---------------cut here---------------end--------------->8--- is caused by "pkg unload symbolic". We fix it by closing file descriptors and waitpid directly in the cleanup function instead of using private/python_ipc_popen2_reset. Fixes gnu-octave#1160. * inst/private/python_ipc_popen2.m: Close file descriptors and waitpid directly in the cleanup function. * inst/private/python_ipc_popen2_reset.m: Remove file.
I'm playing with:
I cannot reproduce locally. I can fix it with
pkg load symbolic; sympref diagnose; syms x; clear all; pkg unload symbolic;
That is adding clear all. Which makes sense. What is supposed to happen?Observed on:
gnuoctave/octave:7.1.0
containerCannot reproduce: locally Fedora 35, Octave 6.4.0 from rpm.
The text was updated successfully, but these errors were encountered: