Skip to content

Revert unintended CR/LF changes to non-3C files that originated in 277d84a5841634783e7d3752707e2a1e4e57a930. #962

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

Merged

Conversation

mattmccutchen-cci
Copy link
Member

We first submitted them in PR #837, but Mandeep noticed
(#837 (review))
and the unintended changes were removed from that PR in
74bfcaf. However, when the squash of
PR #837 was merged with the original commits in
cfc998e, the unintended changes were
incorrectly retained. They got submitted again in the next 3C PR
(#891), and no one noticed that time.

277d84a.

We first submitted them in PR checkedc#837, but Mandeep noticed
(checkedc#837 (review))
and the unintended changes were removed from that PR in
74bfcaf.  However, when the squash of
PR checkedc#837 was merged with the original commits in
cfc998e, the unintended changes were
incorrectly retained.  They got submitted again in the next 3C PR
(checkedc#891), and no one noticed that time.
@mattmccutchen-cci mattmccutchen-cci mentioned this pull request Jan 2, 2021
Copy link
Member

@dtarditi dtarditi left a comment

Choose a reason for hiding this comment

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

@mgrang, do you have any concerns about this change? This change looks good to me.

@dtarditi dtarditi requested a review from mgrang January 12, 2021 06:42
@mgrang
Copy link

mgrang commented Jan 13, 2021

LGTM. @mattmccutchen-cci Could you please verify if the validations are passing on Linux and Windows with this change?

@mattmccutchen-cci
Copy link
Member Author

@mattmccutchen-cci Could you please verify if the validations are passing on Linux and Windows with this change?

I could run the 3C regression tests, but I'm not familiar with how to run your full set of tests (which presumably includes the 3C regression tests). Can you just run them as you would on any other PR?

@mgrang
Copy link

mgrang commented Jan 14, 2021

@mattmccutchen-cci Could you please verify if the validations are passing on Linux and Windows with this change?

I could run the 3C regression tests, but I'm not familiar with how to run your full set of tests (which presumably includes the 3C regression tests). Can you just run them as you would on any other PR?

You can run ninja check-all on Linux and Windows.

@mattmccutchen-cci
Copy link
Member Author

ninja check-all passes on my revert-unintended-crlf-changes-test-merge in our Linux environment. check-all failed to build in our Windows X64 environment with Visual Studio 2019 Community 16.8.4 due to this problem; the same problem occurs without this PR. (check-clang-3c is not affected.) Whatever we blame for the problem, it supposedly no longer occurs with LLVM 11.

I thought I've seen you trigger some process that runs the tests in your environment and posts GitHub status checks on the PR, and I assume that for whatever reason, it isn't affected by the build failure. Can you just use that process?

@mgrang
Copy link

mgrang commented Jan 19, 2021

ninja check-all passes on my revert-unintended-crlf-changes-test-merge in our Linux environment. check-all failed to build in our Windows X64 environment with Visual Studio 2019 Community 16.8.4 due to this problem; the same problem occurs without this PR. (check-clang-3c is not affected.) Whatever we blame for the problem, it supposedly no longer occurs with LLVM 11.

I thought I've seen you trigger some process that runs the tests in your environment and posts GitHub status checks on the PR, and I assume that for whatever reason, it isn't affected by the build failure. Can you just use that process?

We discussed the Windows X64 build issue with @mwhicks1 and he says you should be able to build Windows X64 version without issues.

@mwhicks1
Copy link
Contributor

@mgrang What I said is what you've quoted in the issue: We can successful fun check-clang-3c on Windows X64. This build target requires clang, 3c, and other tools. So in this sense we can "build Windows X64 version."

I don't know what's in check-all that's the problem, but it's not an inability to construct Checked C's clang on Windows X64.

@mgrang
Copy link

mgrang commented Jan 19, 2021

I will go ahead and merge this PR. I will let you know in case our ADO validations break after the merge.

@mgrang mgrang merged commit cfe00ea into checkedc:master Jan 19, 2021
@mattmccutchen-cci
Copy link
Member Author

Thanks @mgrang! Hopefully we get these testing process issues sorted out over time.

@mattmccutchen-cci mattmccutchen-cci deleted the revert-unintended-crlf-changes branch January 19, 2021 22:23
kkjeer pushed a commit that referenced this pull request Jan 19, 2021
)

277d84a.

We first submitted them in PR #837, but Mandeep noticed
(#837 (review))
and the unintended changes were removed from that PR in
74bfcaf.  However, when the squash of
PR #837 was merged with the original commits in
cfc998e, the unintended changes were
incorrectly retained.  They got submitted again in the next 3C PR
(#891), and no one noticed that time.

(cherry picked from commit cfe00ea)
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.

4 participants