Skip to content

Conversation

@tausiffmujawar
Copy link
Contributor

@tausiffmujawar tausiffmujawar commented Jan 5, 2026

Overview

This PR refactors error handling in the Mason package manager so internal functions (masonInit, masonNew) 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

  • tools/mason/MasonInit.chpl: remove local try! { ... } catch e: MasonError { ... exit(1); } wrapper; let MasonError propagate
  • tools/mason/MasonNew.chpl: remove local try/catch wrapper so MasonError propagates (applied earlier)
  • test/mason/masonInit/masonInitTest3.chpl: updated to catch MasonError and print e.message(); added use MasonUtils; and proc main() throws
  • test/mason/masonNewBadName.chpl: updated to catch MasonError and print e.message() (applied earlier)

Rationale

Centralizing error handling at the top-level CLI (tools/mason/mason.chpl) provides:

  • Unit testing: internal functions can be tested without process exit.
  • Consistent error reporting and a single exit-to-user mapping.
  • Separation of concerns: internal code handles logic; CLI handles user-facing exit behavior.

Local testing

$ ./util/start_test test/mason
Test Summary: #Successes = 112 | #Failures = 0 | #Futures = 0 | #Warnings = 0

Branch: mason/issue-28120-fix-mason-error-handling
Commit: 229e4c8
Environment: macOS, CHPL_HOME=/Users/tausiffhussainum/workspace/open-source/chapel

Notes

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 + exit codes continue to be handled by the top-level CLI). Please provide any feedback that can improve the fix. I have made changes in 2 files and based on the feedback i would incrementally make changes to other files as well to use the same pattern to handle errors. @jabraham17 Can you please review the PR.

Copy link
Member

@jabraham17 jabraham17 left a comment

Choose a reason for hiding this comment

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

This PR is a good start, but does not fully resolve #28120, there are many other calls to exit that need to be removed

Also note for DCO, you will need to edit the initial commit to have the DCO, not just add another commit with the DCO

@tausiffmujawar tausiffmujawar force-pushed the mason/issue-28120-fix-mason-error-handling branch from 229e4c8 to 2973503 Compare January 5, 2026 20:04
@tausiffmujawar
Copy link
Contributor Author

@jabraham17 Thanks for the feedback . I have updated the commit history and the DCO check now passes.

Before I proceed further on #28120: would you prefer that I address all remaining "exit()" callsites in this same PR, or should I open additional, smaller PRs to handle them incrementally to reduce verbosity during review? I am fine proceeding either way and will follow your preference.

@jabraham17
Copy link
Member

I have a slight preference for multiple PRs, as the diffs for changing the indent of large blocks of code make reviewing challenging. But, if you put all the changes in this PR I don't mind too much. This PR is good to go as-is (other than resolving the other feedback), but just won't resolve #28120 (and the PR message should be updated accordingly)

Copy link
Member

@jabraham17 jabraham17 left a comment

Choose a reason for hiding this comment

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

Looks good!

Please remove the part about this fixing 82120 from the PR message and I'll merge this

@tausiffmujawar tausiffmujawar changed the title mason: refactor error propagation from masonInit and masonNew (fixes #28120) mason: refactor error propagation from masonInit and masonNew Jan 6, 2026
@tausiffmujawar
Copy link
Contributor Author

Thank you for the review . I removed the “Fixes #28120” wording in the PR description and changed it to “Partially addresses #28120.” Also edited the PR title to remove "fixes #28120" . Please let me know if you’d like any further edits.

@jabraham17 jabraham17 merged commit e760c65 into chapel-lang:main Jan 6, 2026
10 checks passed
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.

2 participants