-
Notifications
You must be signed in to change notification settings - Fork 443
Refactored error propoagation in masonsystem,update and publish files #28339
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
base: main
Are you sure you want to change the base?
Refactored error propoagation in masonsystem,update and publish files #28339
Conversation
Signed-off-by: Tausif Mujawar <[email protected]>
Signed-off-by: Tausif Mujawar <[email protected]>
Signed-off-by: Tausif Mujawar <[email protected]>
Signed-off-by: Tausif Mujawar <[email protected]>
Signed-off-by: Tausif Mujawar <[email protected]>
Signed-off-by: Tausif Mujawar <[email protected]>
…ed after refactor Signed-off-by: Tausif Mujawar <[email protected]>
Signed-off-by: Tausif Mujawar <[email protected]>
|
@jabraham17 Can you please review my PR and let me know if i need to make any changes. I had general doubt, if i should retain "throws" in proc main() in test files or instead use "catch-all" method in test cases. i will update accordingly. |
tools/mason/Mason.chpl
Outdated
| } catch ex: MasonError { | ||
| stderr.writeln(ex.message()); | ||
| exit(1); | ||
| throw ex; |
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 change should not be included. It should be the only exit in Mason. If you wanted to be "pure", you could have it print the error and set the return code
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.
Have made the change as per your comment.
| stderr.writeln(e.message()); | ||
| exit(1); | ||
| throw e; | ||
| } |
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 try/catch is not needed at all
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.
I want to confirm if you want the 'try/catch' removed entirely here so the 'MasonError' propagates, rather than handling it locally? . This doesnt mean i need to revert back to original code as before right.
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.
Correct. There is no need to catch and rethrow the error, just let it propagate
tools/mason/MasonExample.chpl
Outdated
| catch e: MasonError { | ||
| stderr.writeln(e.message()); | ||
| exit(1); | ||
| throw e; |
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 try/catch is not needed at all
tools/mason/MasonPublish.chpl
Outdated
| catch e: MasonError { | ||
| writeln(e.message()); | ||
| exit(1); | ||
| throw e; |
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 try/catch is not needed at all
tools/mason/MasonPublish.chpl
Outdated
| catch e : MasonError { | ||
| writeln(e.message()); | ||
| exit(1); | ||
| throw e; | ||
| } |
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.
same here, try/catch is no longer needed.
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.
and I believe the exit above this should be converted to throws. If there is no test for it, it should be added
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.
I had removed exit(1) as part of refactor logic. Now i removed (not yet committed) try/catch block as well to let error propogate to the caller. So i want to know where i need to add 'throws' in the current context of code (after removing try/catch) or the condition for which i need to "throw".
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.
I believe masonPublish is already marked throws, so just removing the try/catch (and letting the errors propagate) is sufficient
| proc main() throws { | ||
| try { |
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.
instead of proc main() throws, either use try! or add a catch all
test/mason/masonNewTest.chpl
Outdated
| use MasonUtils; | ||
|
|
||
| proc main() { | ||
| proc main() throws { | ||
| var args = ['new', 'Test']; | ||
| masonNew(args); | ||
| try { | ||
| masonNew(args); | ||
| } catch e: MasonError { | ||
| writeln(e.message()); | ||
| } |
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.
since these are not error cases, its much easier to just write try! masonNew
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.
Have made the change as per your comment.
test/mason/masonUpdateTest.chpl
Outdated
| temp.close(); | ||
| lock.close(); | ||
| } catch e: MasonError { | ||
| writeln(e.message()); |
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.
Just use try! updateLock
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.
Have made the change as per your comment.
Signed-off-by: Tausif Mujawar <[email protected]>
Overview
This PR continues the refactor of error handling in the Mason package manager. Internal functions now propagate MasonError to the top-level CLI handler instead of catching and calling exit(1) locally. This centralizes error-to-exit behavior, improves testability, and preserves user-visible behavior.
Changes
[Mason.chpl]
[MasonUpdate.chpl]
[MasonExample.chpl]
[Other Mason source files: MasonSystem.chpl, MasonPublish.chpl, etc.]
[Test files under test/mason] :
[mason-chpl-version-update.chpl]
[masonNewModule.chpl]
[masonNewTest.chpl]
[masonUpdateTest.chpl]
[docPkg.chpl]
[masonHelpTests.good]
[publishOffline.chpl]
Rationale
Centralizing error handling at the top-level CLI (Mason.chpl) provides:
Local testing
$ ./util/start_test test/mason
[Test Summary - 260125.121934]
[Summary: #Successes = 114 | #Failures = 0 | #Futures = 0 | #Warnings = 0 ]
[Summary: #Passing Suppressions = 0 | #Passing Futures = 0 ]
[END]
Branch: mason/issue-28120-fix-third-PR
Environment: macOS, CHPL_HOME=/Users/tausiffhussainum/workspace/open-source/chapel
Notes
All commits include DCO sign-off:
Signed-off-by: Tausif Mujawar [email protected]
This PR continues work on: Refactor mason error handling for better unit testing #28120
Request for reviewers:
Please review the above files and the respective test cases. The change is intended to be behavior-preserving for end users (error messages and exit codes continue to be handled by the top-level CLI). Please provide any feedback that can improve the fix.