llvm / llvm/llvm-project

[clang-format] Solution to #205036 may cause problems on Windows machines with diff in PATH

Open
#205,728 3 comments 0 reactions 0 assignees View on GitHub
clang-format
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

Reference to PR: https://github.com/llvm/llvm-project/pull/205036

This PR may cause issues if a windows machine has diff (and generally some flavor of unix tools) available in the PATH as it will then run that diff.
This will fail under certain circumstances due to the following:
- differences in line endings (when git pulls files from a repo it may or may not add CR characters).
- difference in the operation of clang-format with respect to the environment.

I'm not a clang-format developer so I have not dived into the details here but only run trials on a Jenkins windows pipeline.

I've seen the following:
1. Changing the diff command like below in clang/lib/Format/CMakeLists.txt to use `--strip-trailing-cr` reduces the problem but I still get a few weirdly formatted files in the test and thus failures on Windows (this might be a bug in itself).
```
add_custom_command(OUTPUT check_format_depend_${i}
COMMAND clang-format ${file} | diff --strip-trailing-cr -u ${file} - &&
touch check_format_depend_${i}
VERBATIM
```

2. If, in addition, I add LineEnding in clang/lib/Format/.clang-format and set it like this:
```
BasedOnStyle: clang-format
LineEnding: DeriveCRLF
```
Everything works fine. This seems to hide or get rid of the bug I experience when the LineEnding: LF is in play (from .clang-format in the root llvm directory).

The `--strip-trailing-cr` is necessary on Windows because `stdout` is opened by diff in text mode and will replace the CRLF produced by clang-format with LF.

Someone with more knowledge and involvement in clang-format should probably look into this. My suggestion would be to go for modification 2.

Contributor guide

Open the contributing guide

Research direction

Start with clang/lib/Format/CMakeLists.txt and compare the root .clang-format with clang/lib/Format/.clang-format, then review PR 205036's change. Reproduce the Jenkins Windows scenario with diff in PATH and verify the clang-format check handles line endings and produces stable formatting without failures.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake
Domain
build-system, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.