Revert removal of CRLF from Windows bat files

[friederbluemle]

Summary

This reverts #31128 - For the reasons stated in the thread. Files should have the correct endings in the repo (i.e. Windows .bat CRLF). There is no reason to perform additional conversion with attributes and/or an editorconfig. It was originally fixed in #29792 in August 2020.

⚠️ EDIT 2021-08-31

Commits 85249ca and 13107fa accidentally converted the gradlew.bat files to LF again, resulting in modified files to appear in the working directory:

$ git status -s
 M gradlew.bat
 M packages/react-native-codegen/android/gradlew.bat
 M template/android/gradlew.bat

The reasons why this is happening are explained in detail in the two PRs linked above.

I've added an additional (new) commit to the PR head branch to fix the line endings in all three gradlew.bat files of the repo and rebased it. It should be ready for merge.

CC @cortinico

EDIT 2021-09-02

The additional commit was removed again, but the original one remains.

To test the scenario locally run the following commands on a clean main branch (currently 455433f):

$ rm gradlew.bat
$ git status -s
 D gradlew.bat               # Git shows the file as (D)eleted, as expected
$ git checkout gradlew.bat   # This should restore the file
$ git status -s
 M gradlew.bat               # The file still shows up, now as (M)odified with all line endings changed

The modified file will remain in the working directory until they are committed, or a different branch is force checked out. gradlew.bat files are generated automatically by Gradle (with the correct line endings in the first place). There is no need to special case them and perform line ending conversion using Git and/or editorconfig.

Changelog

[General] [Fixed] - Line endings in Windows files, Git/EditorConfig related conversions

Test Plan

Verify files are stored correctly in the repository (e.g. using the file command).

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,964,746	+0
android	hermes	armeabi-v7a	8,496,901	+0
android	hermes	x86	9,390,971	+0
android	hermes	x86_64	9,358,436	+0
android	jsc	arm64-v8a	10,628,153	+0
android	jsc	armeabi-v7a	9,549,489	+0
android	jsc	x86	10,641,391	+0
android	jsc	x86_64	11,252,557	+0

Base commit: 455433f

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 455433f

[cortinico]

Files should have the correct endings in the repo (i.e. Windows .bat CRLF).

Thanks for your PR @friederbluemle

Those files have been generated automatically by Gradle with the command:

./gradlew wrapper --gradle-version 7.0.2 --distribution-type=all

I'm unsure if removing .gitattributes will actually solve the root cause.

I'm wondering if we would have to manually edit this .bat file whenever we bump the Gradle version.

[friederbluemle]

Thanks for the quick reply @cortinico! The command you posted is correct. There should be no need to manually edit the Gradle Wrapper scripts. Like you said, Gradle will automatically generate the files with the correct line endings.

Can you test the following real quick:

On a clean main branch:

$ rm gradlew.bat
$ git status -s
 D gradlew.bat               # Git shows the file as (D)eleted, as expected
$ git checkout gradlew.bat   # This should restore the file
$ git status -s
 M gradlew.bat               # The file still shows up, now as (M)odified with all line endings changed

[cortinico]

Can you test the following real quick:

Just tested and as you mentioned, the last git status -s shows the gradlew.bat as modified.

I believe that part of the problem is caused by the fact that internally we use mercurial instead of git and phabricator is exporting the git commits to Github.

I'm fine merging your change, but does this mean we would have to do the same whenever we touch the gradlew.bat file?

[facebook-github-bot]

@cortinico has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cortinico]

I'm fine merging your change, but does this mean we would have to do the same whenever we touch the gradlew.bat file?

Hey @friederbluemle Thanks for your PR. I've done a couple of tests and we should be fine merging this 👍

[friederbluemle]

Thanks @cortinico 👍

I believe that part of the problem is caused by the fact that internally we use mercurial instead of git and phabricator is exporting the git commits to Github.

I think Mercurial does not read a .gitattributes files, so there should be no issue one way or the other.

I'm fine merging your change, but does this mean we would have to do the same whenever we touch the gradlew.bat file?

As you've mentioned previously, gradlew.bat is updated/created by Gradle's wrapper task. It will have the correct line endings. There should be no need to touch/edit it manually.

[lunaleaps]

cc @pvinis if there are any concerns?

[pvinis]

seems like we are just "fixing" each other's changes. I am on holidays until end of next week. I could check after. If you want to emrge before, I trust you all.

[friederbluemle]

No rush here.. We can wait for @pvinis to be back from holidays if there are additional questions etc. 👍

[facebook-github-bot]

@cortinico merged this pull request in fc553ed.

[cortinico]

@cortinico merged this pull request in fc553ed.

Hey @pvinis @friederbluemle apologize for the confusion. The diff was merged overnight as it was approved internally. I haven't seen your message early enough to stop the pre-approval @friederbluemle

I believe we can keep it merged as it fixes the local environment for now (e.g. see #32160) but I'm happy to keep on the discussion and find a definitive solution for this 👍

[cortinico]

FYI @yungsters just merged #32178 that updates the gradlew.bat files to use LF only.

I believe that part of the problem is caused by the fact that internally we use mercurial instead of git and phabricator is exporting the git commits to Github.
I think Mercurial does not read a .gitattributes files, so there should be no issue one way or the other.

True, but we use a bot to export from mercurial to git and I believe that bot is having problems handling whitespace chars.

[pvinis]

I know its late, but only now I moved a project to 68. Removing the editorconfig file and instruction seems unnecessary. Say the files are perfectly set in the repo and on everyones inited project, great. Now imagine i need to do an edit on the .bat file, on my mac machine. Maybe it's because of an upgrade, maybe I want to just fix a typo, or anything else in that .bat file. Now I save, and the file and diff is all messed up, because im on a mac. the editorconfig will make sure that the line endings on these files stay the same. gitattrs, i guess we can remove. editorconfig is needed.

What do you think?

[friederbluemle]

Hi @pvinis - Good question. What I don't fully understand - please explain more - why would saving a file have the side effect of converting all line endings in that file? Which editor are you using? I just did a quick test in a RN 68 repo, editing gradlew.bat using Vim, WebStorm, and VS Code. The project did not have an editorconfig, I was testing on macOS as well, and non of the line endings were changed.

[pvinis]

mostly vscode. there's a lot of times/tools/editors that save and "make sure" the endings are right for the os you are in. also, if I have an editorconfig in my home dir, then that will be applied, and if the config is for Mac by default, it will mess up the bat files. it can be fixed by fixing the home config to include bat, but it's just safer.

anyway, I see a lot of resistance, and I have it for my projects, so I don't mind it. I'm only trying to make it less error prone for others 🤷‍♂️.

[cortinico]

@pvinis

Now imagine i need to do an edit on the .bat file, on my mac machine

You should not need to edit that .bat file. That is a gradle generated file and should not be touched. The output of the wrapper tasks already generates files which are valid (from the line-ending pov).

Your argument could potentially hold if you might have other .bat file in your project, but then it's up to you to customize

[pvinis]

I don't believe in "don't touch this file", and if an upgrade has a small change, I usually do it myself, rather than copy paste the whole file anyway, but I get your point. I don't agree, but I get it.

[cortinico]

and if an upgrade has a small change, I usually do it myself,

This specific issue is related to the Gradle Wrapper which must be updated usign this command: https://docs.gradle.org/7.4.2/release-notes.html#upgrade-instructions
So it's an automated bump, there is no copy-n-paste involved here. It will always be an automated step regardless of the size of the updated.