Skip to content
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

testament: don't close Process before reading exit code #1130

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

alaviss
Copy link
Contributor

@alaviss alaviss commented Jan 22, 2024

Summary

On Windows Testament now correctly captures the exit code from commands
(compile, test run, etc).

Details

On platforms like Windows closing a process before reading the exit code
invalidates the exit code, returning 0, which would falsely signify a
success.

Instead, use defer to make sure that the object is kept alive until
the end of scope.

On platforms like Windows this would invalidate the exit code, returning
0 which would falsely signify a success.

Instead, use `defer` to make sure that the object is kept alive until
the end of scope.
@alaviss alaviss added bug Something isn't working utils Internal compiler utilities labels Jan 22, 2024
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

LGTM, I left a soft suggestion explaining why we defer in a comment.

I also updated the PR message, please take a read before merging, thanks.

testament/testament.nim Show resolved Hide resolved
@alaviss
Copy link
Contributor Author

alaviss commented Jan 22, 2024

/merge

Copy link

Merge requested by: @alaviss

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

@chore-runner chore-runner bot enabled auto-merge January 22, 2024 23:07
@chore-runner chore-runner bot added this pull request to the merge queue Jan 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2024
@alaviss alaviss added this pull request to the merge queue Jan 23, 2024
Merged via the queue into nim-works:devel with commit 99a0ca7 Jan 23, 2024
19 checks passed
@alaviss alaviss deleted the testament-close branch January 23, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working utils Internal compiler utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants