Skip to content

[llvm-rc] Handle Windows line endings in tag-html test #113000

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
merged 1 commit into from
Oct 22, 2024

Conversation

dpaoliello
Copy link
Contributor

@dpaoliello dpaoliello commented Oct 18, 2024

The tag-html.test has been failing for me and in CI if Git happens to decide to check out the baseline file with Windows line endings.

This fix for this is to call tr to strip Windows newlines when copying the baselines files to the test output directory before embedding them.

@dpaoliello dpaoliello changed the title Handle Windows line endings in tag-html test [llvm-rc] Handle Windows line endings in tag-html test Oct 18, 2024
@dpaoliello dpaoliello requested review from ldrumm and jayfoad October 18, 2024 23:25
@dpaoliello
Copy link
Contributor Author

@llvm/pr-subscribers-windows can I believe get a review for this: it fixes the last failure we've been seeing in the PR builds for Windows.

@mstorsjo
Copy link
Member

mstorsjo commented Oct 21, 2024

@llvm/pr-subscribers-windows can I believe get a review for this: it fixes the last failure we've been seeing in the PR builds for Windows.

Are you seeing those issues still if you rebase your branch to a recent base version?

See #86318 for the source of those issues - that was reverted in e669bbb. And there's a chance that it can end up reapplied with .gitattributes enforcing checking out these files with LF newlines.

That said, it would of course be nice if the tests pass even if checked out with CRLF, so I guess this fix can be acceptable for that. It's not really ideal, but could work.

A different alternative could be to mark specifically those Inputs files as explicitly requiring LF newlines. Earlier, all of these Inputs were marked as binary, but that was pretty annoying for any diff operation, so I changed that in e46822e. But I guess it could work to add a similar annotation for specifically those webpage*.html to enforce LF newlines on it, always?

@dpaoliello
Copy link
Contributor Author

Are you seeing those issues still if you rebase your branch to a recent base version?

See #86318 for the source of those issues - that was reverted in e669bbb. And there's a chance that it can end up reapplied with .gitattributes enforcing checking out these files with LF newlines.

I'm still seeing failures in the CI machines - even with that change reverted, I don't think that it's forcing Git to re-create the files with the correct line endings.

Just looking through recent PRs with build failures, I see https://buildkite.com/llvm-project/github-pull-requests/builds/111934#0192b0c1-cdad-4ace-936e-93d0626d903d which has the revert in its commit history, but is still failing due to this issue (and the other which I recently merged a fix for).

@mstorsjo
Copy link
Member

Are you seeing those issues still if you rebase your branch to a recent base version?
See #86318 for the source of those issues - that was reverted in e669bbb. And there's a chance that it can end up reapplied with .gitattributes enforcing checking out these files with LF newlines.

I'm still seeing failures in the CI machines - even with that change reverted, I don't think that it's forcing Git to re-create the files with the correct line endings.

Just looking through recent PRs with build failures, I see https://buildkite.com/llvm-project/github-pull-requests/builds/111934#0192b0c1-cdad-4ace-936e-93d0626d903d which has the revert in its commit history, but is still failing due to this issue (and the other which I recently merged a fix for).

Ah, right, because it seems that the CI machines only updates the existing workdir, it doesn't do a full new checkout. So it seems like the CI machines have started up right in the window when the problematic gitattributes were in effect, and any attempts to fix it via gitattributes has no effect now. See https://discourse.llvm.org/t/windows-premerge-buildbot-broken-for-5-days/82571/4 for discussion around this as well.

I presume this fix is good enough for now; when we get the workers checkouts flushed and re-checked out with LF newlines again, I'd prefer to go with the solution from #113222, but for now this is probably fine.

Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

LGTM

@ldrumm
Copy link
Contributor

ldrumm commented Oct 22, 2024

Thanks for the fix! I agree that ideally we shouldn't have to perform shenanigans for each test, but if this cleans up my mess, then I think we should merge it

@dpaoliello dpaoliello merged commit 2fdf49d into llvm:main Oct 22, 2024
8 checks passed
@dpaoliello dpaoliello deleted the taghtml branch October 22, 2024 16:54
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