This is an archive of the discontinued LLVM Phabricator instance.

clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files
AbandonedPublic

Authored by amaiorano on Jan 6 2017, 3:51 PM.

Details

Reviewers
klimek
Summary

Certain tests expect their input files to have LF line endings, while others expect CRLF. On Windows, when using Git with core.autocrlf=true, these tests fail because all source files are checked out with CRLF line endings. This change makes sure that these files are not converted at all on checkout.

Diff Detail

Event Timeline

amaiorano updated this revision to Diff 83451.Jan 6 2017, 3:51 PM
amaiorano retitled this revision from to clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files.
amaiorano updated this object.
amaiorano added reviewers: malcolm.parsons, ioeric.
amaiorano added a subscriber: cfe-commits.
ioeric edited reviewers, added: klimek; removed: ioeric.Jan 9 2017, 12:59 AM
ioeric added a subscriber: ioeric.

I'm not entirely sure about this change... looping in Manuel.

malcolm.parsons resigned from this revision.Jan 9 2017, 3:58 AM
malcolm.parsons removed a reviewer: malcolm.parsons.

Which tests?
How does this interact with svn's eol-style property?

Resigning as not something I know about.

Hello, sorry for the lack of details here. I will re-run the tests later and report here the tests that are failing along with output.

The main issue is that when using Git on Windows, by default the installer will set core.autocrlf to true, which is a Git config variable that tells Git to automatically convert line endings to CRLF for text files on checkout, and back to LF on staging. Some of the tests expect LF line endings - for instance, when specifying absolute offsets for replacements, these offsets are incorrect when there are extra "CR" bytes in the file, which end up making the tests fail since the output doesn't match the expected output.

Note that on Linux, I believe core.autocrlf isn't usually set, so it defaults to false (could someone verify this for me?). Since files in Git are usually 'LF', this works fine for Linux users. On Windows, you usually want the CRLF line endings on source files because not all Windows text editors are good at being consistent with line endings.

The solution I propose here is to add a single .gitattributes file that specifies that all .h and .cpp files under clang/tools/extra/test should be checked out with whatever line endings they were checked in with. In other words, don't convert line endings at all for these files. This means existing test source files that have LF will continue to have LF when checked out in Git, or if they have CRLF (which is the case for crlf.cpp), it will be checked out with CRLF line endings.

Another consequence of this change is that if a Windows developer decides to add a new test, they'd create a test source file with CRLF line endings, and this file would be committed with these CRLF line endings. This would work fine on Linux since the file would be checked out as-is as well (with CRLF line endings), and the test would run fine. However, without this change, the Windows developer would write a test that works fine on Windows, then when staging the file, the CRLF would be converted to LF and the test would fail on Linux.

Alternative solutions:

  1. Instead of applying the no eol conversion to ALL .h/.cpp under test, we could add explicit entries to the .h/.cpp files that specifically require them for their tests (those that specify offsets for replacements, for e.g.)
  2. Instead of a single .gitattributes file at the root of the extra/ directory, we can add one to each test directory that requires it, specifying the exact files in that test directory that needs to not apply eol conversions.

Again, I will post the specific failures as soon as I get the chance.

Last night, I realized that this problem extends to the clang/tests as well, not just those in clang-tools-extra. Is there someone who knows Windows, Git, and the testing pipeline that can weigh in more on this topic?

amaiorano abandoned this revision.May 5 2017, 6:56 AM