llvm-beanz wrote:

> I don't know if the pre-commit testing guarantees that. Configuration 
> settings will permit the files to be checked out in either Unix (`\n`) or 
> Windows (`\r\n`) line-endings.

Today on Windows you basically have to check out LLVM as unix line endings. 
There are a bunch of tests that fail if you don't. Hopefully this is a step 
toward fixing that (which is why I'm in favor of this).

> If the builders are all configured to run in Unix line endings we lose that 
> coverage. 

I suspect the Windows bots are basically all configured that way today. Our 
instructions tell people they need to disable `autocrlf` (see: 
https://llvm.org/docs/GettingStarted.html#getting-the-source-code-and-building-llvm),
 and not disabling it causes problems for users still (see this message from 
today: 
https://discord.com/channels/636084430946959380/745925509971705877/1235913415902629958).

>While I understand that this change only changes how the LLVM sources are 
>stored, the issue is that the LLVM sources include the _tests_ directory. 
>These need to be tested in various manners to ensure that we test how we 
>handle the different inputs (in clang). One option might be to exclude the 
>tests directory from the line ending conversion if we cannot verify that we 
>have tests in both formats.

I see two issues with this:

1) because the repo doesn't work with autocrlf today, we actually can't rely on 
testing on different platforms to provide this coverage.
2) We shouldn't be relying on git settings to get this coverage anyways. We 
should have tests that require specific line endings, and we should use 
gitattributes to ensure that we run those tests regardless of the user's git 
settings.

> While, yes, it is possible to get the wrong behaviour currently, if the user 
> configures git explicitly for a line-ending, I would expect them to be aware 
> of that. The use of `.gitattributes` means that their defaults are not 
> necessarily honoured and thus this can catch them by surprise.

I think today not only is it possible to get the wrong behavior, if you're 
developing on Windows Git's default behavior causes the wrong behavior for 
LLVM. Which often means that contributors (especially new contributors) have no 
idea that they're doing something wrong.

I don't know that this PR is perfect, but IMO it is a huge step in the right 
direction to allow Windows contributors to build, test and contribute to LLVM 
using the default configurations of the OS and tools.

https://github.com/llvm/llvm-project/pull/86318
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to