Skip to content

set exit code 0 at the end#403

Closed
javierggt wants to merge 1 commit intomasterfrom
exit-code
Closed

set exit code 0 at the end#403
javierggt wants to merge 1 commit intomasterfrom
exit-code

Conversation

@javierggt
Copy link
Copy Markdown
Contributor

@javierggt javierggt commented Jan 9, 2023

Description

This PR sets the exit code to 0 at the end of execution.

I can confirm that the current master version return code is not zero. This causes test_regress.sh to fail. Someone who is more familiar with perl should check this.

Interface impacts

None

Testing

Unit tests

  • No unit tests

Functional tests

I checked that the program runs and returns the given value using the same command as in test_regress.sh:

starcheck -dir $SKA/data/ska_testr/test_loads/2019/MAY2019/oflsa

I did not check an error elsewhere to make sure the error is properly set. I would need to know what error to check for.

@jeanconn
Copy link
Copy Markdown
Contributor

jeanconn commented Jan 9, 2023

I think what we want is non-zero exit code if the perl exits via any of the "die" statements and a zero exit code otherwise. Not sure if setting the exit code at the end does that and/or if the END block should preserve and re-spit-out the exit code if it gets there via die. Will look at this some more.


=cut

exit 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be at the end of the END {} block above the docs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with that, I think, is that END still runs after an unexpected "die", so you get zero exit status even on a failure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point. Sorry, never mind.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe

END {
   my $exit_status = $?;
   ...
   exit($exit_status);
}

@jeanconn
Copy link
Copy Markdown
Contributor

jeanconn commented Jan 9, 2023

Also this code does work to output zero but it didn't start with master with the merges that are the problem.

@javierggt
Copy link
Copy Markdown
Contributor Author

Also this code does work to output zero but it didn't start with master with the merges that are the problem.

That's right. It started at another commit. I rebased it.

@javierggt
Copy link
Copy Markdown
Contributor Author

javierggt commented Jan 9, 2023

Now looking better at the code it is obvious which statement is the one that sets the RC code (to 9):

	kill 9, $pid;                    # must it be 9 (SIGKILL)?
        my $gone_pid = waitpid $pid, 0;  # then check that it's gone

@jeanconn
Copy link
Copy Markdown
Contributor

jeanconn commented Jan 9, 2023

I saw that nine but I wasn't 100% sure it was setting the exit code. Either way, my suggestion is in #404 . It seems to work for the success case and for the simplest "die" case of "-dir" not being a products directory.

@jeanconn
Copy link
Copy Markdown
Contributor

jeanconn commented Jan 9, 2023

I was reading up more on appropriate handling of errors around END but maybe we don't care about best practices.

@jeanconn
Copy link
Copy Markdown
Contributor

jeanconn commented Jan 9, 2023

Superseded by #404

@jeanconn jeanconn closed this Jan 9, 2023
@jeanconn jeanconn deleted the exit-code branch July 13, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants