Page MenuHomePhabricator

fix check-clang-tools tests that fail due to Windows CRLF line endings
ClosedPublic

Authored by poelmanc on Feb 28 2021, 12:41 AM.

Details

Summary

Running check-clang-tools on Windows produces 5 test failures:

Failed Tests (5):
  Clang Tools :: clang-apply-replacements/ClangRenameClassReplacements.cpp
  Clang Tools :: clang-apply-replacements/basic.cpp
  Clang Tools :: clang-apply-replacements/format.cpp
  Clang Tools :: clang-move/move-used-helper-decls.cpp
  Clang Tools :: clang-tidy/infrastructure/export-diagnostics.cpp

Four of these failures are simply due to fixed character position offsets differing on Windows versus Linux, since Windows line endings take up two characters instead of one:

  • clang-apply-replacements/ClangRenameClassReplacements.cpp runs clang-rename -offset=254
  • clang-apply-replacements/Inputs/basic/file[12].yaml specify e.g. FileOffset: 148 and Offset: 298
  • clang-apply-replacements/Inputs/format/{no,yes}.yaml specify e.g. FileOffset: 94 and Offset: 94
  • clang-tidy/infrastructure/export-diagnostics.cpp specifies e.g. CHECK-YAML-NEXT: FileOffset: 30

(The move-used-helper-decls.cpp failure seems more complex; clang-move adds a blank line after void HelperFun1() {} when clang-move/Inputs/helper_decls_test.cpp has LF line endings, but does not add a blank line when the input files has CRLF line endings. That difference in behavior seems like it may be an actual bug, but I have yet to track it down.)

Do other folks developing on Windows see this? Performing a git checkout with Linux (LF) or "as-is" line endings would make the tests pass, but the underlying clang tools need to work on on input files with various line ending types, so it seems best to test the code against various line ending types. For example, last year I found a clang-tidy fix that chomped a CRLF line ending in half by implicitly assuming a line ending to be one byte, and the above clang-move issue could be similar. Ideally the Windows BuildKite job should test input files with CRLF line endings; since the above tests pass, I'd guess it's checking out test files only with LF line endings?

  • I considered changing export-diagnostics.cpp to e.g. CHECK-YAML-NEXT: FileOffset: 3[01] to accept either, but that would relax the test on Linux - we really want exactly 30 for LF line endings, and exactly 31 for CRLF line endings.
  • An ideal fix might somehow enable different offset numbers depending on the input file line endings, but that seems like a big change.
  • Instead this patch adds a .gitattributes file to specify Linux LF line endings only for the above files that use -offset, FileOffset and Offset in their tests, CRLF line endings for the one crlf.cpp test, and auto for everything else.

I'm open to any other thoughts, ideas, or hints about testing clang tools on inputs with various line ending types.

Diff Detail

Event Timeline

poelmanc created this revision.Feb 28 2021, 12:41 AM
poelmanc requested review of this revision.Feb 28 2021, 12:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2021, 12:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I have run into this line ending problem with python scripts as well.
If you follow the advice on the "getting started" pages, then you will
have checked out the entire repo with Unix line endings, but when you
run python scripts like add_new_check.py for clang-tidy, it writes out
the files with native line endings and ends up flagging the entirety of
the modified file as changing due to the flip-flopping of line endings.

I have an open bug on this,
but I think we need to have a wider discussion in order to reach a
more global fix.

Your limited scope fix looks good to me.

This revision is now accepted and ready to land.Jan 10 2022, 9:57 AM

Do you have commit access or do you need someone to push this change for you?

Sorry I somehow missed the acceptance back on 10 Jan. I lack commit access, it would be great if you could push for me, thanks!

Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 2:24 PM

Hello. I noticed this was applied to the base /test directory in the llvm repo: https://github.com/llvm/llvm-project/tree/main/test
Was it intended be in clang-tools-extra: https://github.com/llvm/llvm-project/tree/main/clang-tools-extra/test? I'm not sure it will do much where it is.

bcain added a subscriber: bcain.Mar 19 2022, 9:00 PM

Hello. I noticed this was applied to the base /test directory in the llvm repo: https://github.com/llvm/llvm-project/tree/main/test
Was it intended be in clang-tools-extra: https://github.com/llvm/llvm-project/tree/main/clang-tools-extra/test? I'm not sure it will do much where it is.

It's almost certainly the case.

@LegalizeAdulthood this commit should be reverted. If it makes sense, it could be re-applied to clang-tools-extra/test/.

Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2022, 9:00 PM

I'm not sure it will do much where it is.

It won't, to quote gitattributes(5):

When deciding what attributes are assigned to a path, Git consults $GIT_DIR/info/attributes file (which has the highest precedence), .gitattributes file in the same directory as the path in question, and its parent directories up to the toplevel of the work tree (the further the directory that contains .gitattributes is from the path in question, the lower its precedence).

Obviously test/ is not a parent directory of clang-tools-extra/test.

If it makes sense, it could be re-applied to clang-tools-extra/test/.

I thought that maybe lit would try to run it as a test then, but llvm-lit --show-tests clang-tools-extra/test/ doesn't list it if I move it there. Specifically llvm-lit --show-tests clang-tools-extra/test/.gitattributes prints:

llvm-lit: [...]/llvm/utils/lit/lit/discovery.py:176: error: 'Clang Tools :: .gitattributes' would not be run indirectly: change name or LIT config(e.g. suffixes or standalone_tests variables)
1 errors, exiting.

Indeed, in llvm/utils/lit/lit/formats/base.py:

class FileBasedTest(TestFormat):
    def getTestsInDirectory(self, testSuite, path_in_suite,
                            litConfig, localConfig):
        source_path = testSuite.getSourcePath(path_in_suite)
        for filename in os.listdir(source_path):
            # Ignore dot files and excluded tests.
            if (filename.startswith('.') or
                filename in localConfig.excludes):
                continue

It also applies the attributes correctly:

> git check-attr -a clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp: text: set
clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp: eol: crlf

Previously that printed nothing.

So I went ahead and just fixed this in ac5f7be6a8688955a282becf00eebc542238a86b.

thakis added a subscriber: thakis.May 3 2022, 6:15 AM

So I went ahead and just fixed this in ac5f7be6a8688955a282becf00eebc542238a86b.

As far as we can tell, this broke clang-move/move-used-helper-decls.cpp on our windows bot. D124563 then fixed it again. (So all good now, just for the record.)

https://crbug.com/1319491 was our upstream bug.