-
Notifications
You must be signed in to change notification settings - Fork 897
Update the debugger support so it properly launches under a debugger, and supports attach to a running job #3709
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
Conversation
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 looks good to me. I had a very minor comment update if you get to it.
orte/tools/orterun/orterun.c
Outdated
@@ -2414,7 +2414,7 @@ static void setup_debugger_job(void) | |||
*/ | |||
orte_plm_base_create_jobid(debugger); | |||
/* set the personality to ORTE */ |
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.
Should probably update the comment to ompi
instead of orte
to reflect the line that follows.
… and supports attach to a running job Fixes #3660 Signed-off-by: Ralph Castain <[email protected]>
@jjhursey Done! |
This looks good to me, thanks! |
/* set the personality to ORTE */ | ||
debugger->personality = strdup("orte"); | ||
/* set the personality to OMPI */ | ||
debugger->personality = strdup("ompi"); |
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.
@rhc54 Just curious -- why change this to "ompi" inside of orterun
?
/* set the personality to ORTE */ | ||
debugger->personality = strdup("orte"); | ||
/* set the personality to OMPI */ | ||
debugger->personality = strdup("ompi"); |
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.
why the change here?
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.
Because you folks removed the "orte" schizo component in 2.x, so we have to default to the "ompi" one 😄
Do we need this fix in v3.0.x? |
It's already in master and 3.x - the problem was the prior branches |
Thanks to Greg Lee for the report
Fixes #3660
Signed-off-by: Ralph Castain [email protected]