Skip to content

Conversation

@kkysen
Copy link
Contributor

@kkysen kkysen commented Aug 8, 2025

This reverts commit 6c689e1.

This broke CI (c2rust-testsuite), which wasn't caught because CI wasn't run on PRs not directly into master.

…the default for the CLI, too"

This reverts commit 6c689e1.

This broke CI (`c2rust-testsuite`), which wasn't caught
because CI wasn't run on PRs not directly into `master`.

* #1312 fixes this.
@fw-immunant
Copy link
Contributor

Making PRs into branches other than master always confuses me. It means that reviewing requires considering the diff of the PR and the diff between the PR's destination and master.

@fw-immunant
Copy link
Contributor

fw-immunant commented Aug 8, 2025

Shouldn't we revert the whole PR, not just one commit? Based on the CI run it looks like we have actual breakage that we should have fixed before merging the --translate-const-macros changes; the particular commit here still looks correct to me in context.

Copy link
Contributor Author

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Shouldn't we revert the whole PR, not just one commit? Based on the CI run it looks like we have actual breakage that we should have fixed before merging the --translate-const-macros changes; the particular commit here still looks correct to me in context.

The breakage is fundamentally from an earlier commit in the PR, but it's less churn for me to just revert this one, fix it, and then un-revert it. Can I merge this PR now? Might not be ideal, but it should just get things done quickly.

@kkysen
Copy link
Contributor Author

kkysen commented Aug 9, 2025

I'm going to merge this so CI stops being broken.

@kkysen kkysen merged commit 45dd238 into master Aug 9, 2025
6 checks passed
@kkysen kkysen deleted the kkysen/revert-broken-ci branch August 9, 2025 03:26
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