Remove session-specific directory if module is given Ctrl-C#5739
Merged
PaulWessel merged 28 commits intomasterfrom Oct 12, 2021
Merged
Remove session-specific directory if module is given Ctrl-C#5739PaulWessel merged 28 commits intomasterfrom
PaulWessel merged 28 commits intomasterfrom
Conversation
Contributor
|
Doing multiple tests starting a session and I don't know if this is of any help, but just in case; other outcomes include: |
Member
Author
|
Please try a few without the process substitution which is a separate process running. Just so we can compare |
Member
Author
|
Basically, gmt has no way of knowing that you kill the makecpt command with Ctrl-C, just that something suddenly got removed in the middle of another module... |
Contributor
|
Seems to work better with no process substitution. Still, again, some problems with the terminal not being responsive. And other cases, just to give an impression what the user may see: Session was not deleted in this case, so: Need to clear: |
Member
Author
|
Interesting. Hard to debug these things in the debugger... |
Member
Author
|
Let me know if my changes make any difference. |
Member
Author
|
hi @anbj the latest also calls GMT_Destroy_Session, perhaps that prevents some of the messages you get, but not sure. It seems to report errors after I call exit, which is odd. |
Contributor
|
(I could not Still getting a good mix. But I think is works better now. |
Member
Author
|
I will test this on my Linux laptop today so maybe I can learn some more. Those strange message from malloc sysmalloc is not GMT so I wonder if exit is a macro on your Linux or similar. exit should just exit, not fuss... |
Contributor
|
Maybe. I'm on Debian 11 Bullseye, if that helps. And, please don't hesitate to say stop if this is getting too hairy and taking up too much time. |
Member
Author
|
Could @joa-quim perhaps check if this works on WIndows? (Compile, run, CTRL-C the program). We should be able to do Ctrl-C checking on all OS even if the backtrace stuff will be limited to the *nix case. Right now it is all or nothing and Windows is excluded. May even be more important to do this on WIndows since the PID is hardwired to 1. |
Member
Author
|
Hi @anbj - the latest commit works pretty good for me on CentOS. I made some changes that possibly stabilizes things for you as well. let me know. |
Contributor
|
Yes, works better now. Some snacks though: (Need to clean this manually now with |
Member
|
It does no fix the issue that seems to be that the |
Member
Author
|
I found an issue in the CMakeLists.txt file: The libgmt list includes gmt_common_sighandler.c twice instead of both c and h. That was probably it all along. Please check again. |
Member
|
Not yet. Still same linking error. |
Member
Author
|
Assuming you fully cleared byte build dir and rebuilt, this makes no sense. Can you think of anything that is different her w.r.t. that function relative to all other GMT exported functions? |
Member
|
Don't know. or some reason the |
Member
Author
|
Can you run a win command to see the list of functions inside the gmt.dll
and check if it is there or not? Next would be to add some #message
statements in the areas where that function is declared and prototyped to
see if/why the compiler skips those sections.
On September 30, 2021 at 11:04:42 AM, Joaquim ***@***.***) wrote:
Don't know. or some reason the sig_handler_win32 function does not land in
the gmt.dll
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#5739 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGJ7IX4OSH5RFUZRFAESPYDUETGGVANCNFSM5DV3HSOQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Member
|
I can SEE that it's not in the dll and functions are not being skipped. Many attempts to change things gave compiling errors. So it's being compiled. But it just vanishes. |
Member
Author
|
@joa-quim, I have made a few changes to where prototypes are listed etc, perhaps this will help (or not). |
Member
|
Yes, now it worked But it must take care that |
joa-quim
approved these changes
Oct 12, 2021
Member
Author
Do you mean should this be under #ifndef WIN32? |
Member
Author
|
I think the answer to my question is yes since those are Unix sys includes. I will add the ifndef. |
Member
|
That is somehow already under an |
Member
Author
|
OK, so now it cannot be set under WIN32. I will merge this PR once the checks completes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See #5730 for background. This PR reinstates a check for SIG_INT but the sig_handler has been changed to simply call gmtlib_terminate_session () which will delete the current session directory so that we do not leave useless junk behind. Closes #5730.
We need to test this a bit though. Again, not supported on Windows unless @joa-quim knows how it works there. Run a modern mode command and hit Ctrl-C before it finishes. You should see a message like this
The session directory should have been removed and you can run the same command (without Ctrl-C) and it will not complain about being in a modern mode session already.
This PR also makes a few changes in gmt.c by now listening for SIG_INT and then doing the above if caught.
Note: If you are running a modern mode script and you hit Ctrl-C then there are complications:
We cannot control those things, but we can control interrupting a module.
We could also explain all of this, the way we ensure session continuity etc to preempt dumb things like running gmt begin in one terminal and running the next command in another terminal, or putting a gmt modern mode script in the background (prompt$ my_long_script.sh &) and then run another script in the same terminal. I think that will cause interference.