Added .gitattributes to manage end-of-line checks for Windows/*nix systems #1638

reta · 2021-12-01T17:43:37Z

Signed-off-by: Andriy Redko andriy.redko@aiven.io

Description

There is a difference between end of line CRLF vs LF between Windows/*nix systems which causes spotless checks to fail. The suggested solution is to use .gitattributes , see please diffplug/spotless#23

Linux:

$ ./gradlew spotlessJavaCheck
...
> Configure project :qa:os
Cannot add task 'destructiveDistroTest.docker' as a task with that name already exists.
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.3
  OS Info               : Linux 5.13.0-22-generic (amd64)
  JDK Version           : 11 (Ubuntu JDK)
  JAVA_HOME             : /usr/lib/jvm/java-11-openjdk-amd64
  Random Testing Seed   : 2A834AAD373C92B6
  In FIPS 140 mode      : false
=======================================

BUILD SUCCESSFUL in 2s
169 actionable tasks: 169 up-to-date

Windows:

$ ./gradlew.bat spotlessJavaCheck

> Configure project :plugins:repository-hdfs
hdfsFixture unsupported, please set HADOOP_HOME and put HADOOP_HOME\bin in PATH

> Configure project :qa:os
Cannot add task 'destructiveDistroTest.docker' as a task with that name already exists.
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.3
  OS Info               : Windows 10 10.0 (amd64)
  JDK Version           : 11 (Amazon Corretto JDK)
  JAVA_HOME             : C:\Program Files\Amazon Corretto\jdk11.0.11_9
  Random Testing Seed   : EA3DA7454331063
  In FIPS 140 mode      : false
=======================================

BUILD SUCCESSFUL in 7s
169 actionable tasks: 169 up-to-date

Issues Resolved

Closes #1637

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

opensearch-ci-bot · 2021-12-01T17:44:04Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2021-12-01T17:45:59Z

✅ Gradle Wrapper Validation success 6e078d3293a62a20b9867fd360901fe09ecfbebd

opensearch-ci-bot · 2021-12-01T17:50:55Z

✅ Gradle Precommit success 6e078d3293a62a20b9867fd360901fe09ecfbebd

opensearch-ci-bot · 2021-12-01T18:15:30Z

❌ Gradle Check failure 6e078d3293a62a20b9867fd360901fe09ecfbebd
Log 1281

Reports 1281

opensearch-ci-bot · 2021-12-01T18:24:12Z

✅ Gradle Wrapper Validation success 82f707c93d9ed1ccba0b81a77c569c16f0aeb1e2

opensearch-ci-bot · 2021-12-01T18:28:37Z

✅ Gradle Precommit success 82f707c93d9ed1ccba0b81a77c569c16f0aeb1e2

owaiskazi19 · 2021-12-01T18:35:56Z

@reta Thanks for the PR. Coming from #1637, can you let me know what result/issue we get on Windows after running ./gradlew.bat :benchmarks:spotlessApply? I'm just trying to understand if we need .gitattributes.

opensearch-ci-bot · 2021-12-01T18:51:33Z

❌ Gradle Check failure 82f707c93d9ed1ccba0b81a77c569c16f0aeb1e2
Log 1282

Reports 1282

dblock · 2021-12-01T19:06:18Z

+1 on @owaiskazi19 question, we should have consistent CR/LFs in code regardless of whether someone edited a file on Windows or *nix

opensearch-ci-bot · 2021-12-01T19:07:57Z

✅ Gradle Wrapper Validation success ebc8654bcec9cfaf79657365edb9157b497812af

opensearch-ci-bot · 2021-12-01T19:11:19Z

✅ Gradle Precommit success ebc8654bcec9cfaf79657365edb9157b497812af

reta · 2021-12-01T19:12:34Z

@dblock @owaiskazi19 yes, sure:

$ ./gradlew.bat :benchmarks:spotlessApply                                                                             
                                                                                                                      
> Configure project :plugins:repository-hdfs                                                                          
hdfsFixture unsupported, please set HADOOP_HOME and put HADOOP_HOME\bin in PATH                                       
                                                                                                                      
> Configure project :qa:os                                                                                            
Cannot add task 'destructiveDistroTest.docker' as a task with that name already exists.                               
=======================================                                                                               
OpenSearch Build Hamster says Hello!                                                                                  
  Gradle Version        : 7.3                                                                                         
  OS Info               : Windows 10 10.0 (amd64)                                                                     
  JDK Version           : 11 (Amazon Corretto JDK)                                                                    
  JAVA_HOME             : C:\Program Files\Amazon Corretto\jdk11.0.11_9                                               
  Random Testing Seed   : 89C568BCDCA3C3D8                                                                            
  In FIPS 140 mode      : false                                                                                       
=======================================                                                                               
                                                                                                                      
BUILD SUCCESSFUL in 6s                                                                                                
17 actionable tasks: 3 executed, 14 up-to-date

Here is the diff:

$ git diff
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/fs/AvailableIndexFoldersBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/indices/breaker/MemoryStatsBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/routing/allocation/AllocationBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/routing/allocation/Allocators.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/search/aggregations/TermsReduceBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/search/aggregations/bucket/terms/LongKeyedBucketOrdsBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/search/aggregations/bucket/terms/StringTermsSerializationBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/time/DateFormatterBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/time/DateFormatterFromBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/benchmark/time/RoundingBenchmark.java.
The file will have its original line endings in your working directory
warning: CRLF will be replaced by LF in benchmarks/src/main/java/org/opensearch/common/RoundingBenchmark.java.
The file will have its original line endings in your working directory

PS: running it on main

owaiskazi19 · 2021-12-01T19:21:53Z

Thanks for the output. Since ./gradlew.bat :benchmarks:spotlessApply is successful gradle check on Windows should not break at least because of Spotless or is there anything else I'm missing here?

opensearch-ci-bot · 2021-12-01T19:38:32Z

❌ Gradle Check failure ebc8654bcec9cfaf79657365edb9157b497812af
Log 1286

Reports 1286

reta · 2021-12-01T19:48:00Z

Thanks for the output. Since ./gradlew.bat :benchmarks:spotlessApply is successful gradle check on Windows should not break at least because of Spotless or is there anything else I'm missing here

@owaiskazi19 I think you are mistaken spotlessApply for spotlessJavaCheck:
./gradlew.bat :benchmarks:spotlessApply - does reformat the code (not part of the check or build)
./gradlew.bat :benchmarks:spotlessJavaCheck - does check the format of the code (part of the check or build)

The task which fails is spotlessJavaCheck, not spotlessApply

opensearch-ci-bot · 2021-12-01T19:50:26Z

✅ Gradle Wrapper Validation success b581e54232a426539d2101e1894b7c86c5b7d8eb

owaiskazi19 · 2021-12-01T19:57:45Z

Sorry if I was not clear @reta. I meant since we had a successful run of SpotlessApply, can't we just raise a PR for that formatted change instead of adding .gitattributes file?
Wouldn’t that change will help for a successful SpotlessJavaCheck which in turns have a successful gradle check?

reta · 2021-12-01T20:00:07Z

Sorry if I was not clear @reta. I meant since we had a successful run of SpotlessApply, can't we just raise a PR for that formatted change instead of adding .gitattributes file?

I suspect if we fix Windows formatting, than spotlessJavaCheck will fail on *nix :) The recipe with .gitattributes comes from spotless maintainers: diffplug/spotless#39

opensearch-ci-bot · 2021-12-01T20:03:03Z

✅ Gradle Precommit success b581e54232a426539d2101e1894b7c86c5b7d8eb

opensearch-ci-bot · 2021-12-01T20:14:16Z

❌ Gradle Check failure b581e54232a426539d2101e1894b7c86c5b7d8eb
Log 1287

Reports 1287

owaiskazi19 · 2021-12-01T20:23:57Z

Sorry if I was not clear @reta. I meant since we had a successful run of SpotlessApply, can't we just raise a PR for that formatted change instead of adding .gitattributes file?

I suspect if we fix Windows formatting, than spotlessJavaCheck will fail on *nix :) The recipe with .gitattributes comes from spotless maintainers: diffplug/spotless#39

I'm not sure if it will break on *nix for this specific case. Can you give it a try?
Also, if we plan to go with .gitattributes, don't we have to include lineEndings 'GIT_ATTRIBUTES' as mentioned here: https://github.com/diffplug/spotless/blob/9aefb65262685a9959360470b070e2e649f038eb/README.md#custom-rules. @dblock WDYT of .gitattributes as it's recommended here: diffplug/spotless#39 (comment)?

opensearch-ci-bot · 2021-12-01T20:26:00Z

✅ Gradle Wrapper Validation success ebf5367908e461e7747f7c612cd125902eb74b86

opensearch-ci-bot · 2021-12-01T20:29:23Z

✅ Gradle Precommit success ebf5367908e461e7747f7c612cd125902eb74b86

reta · 2021-12-01T20:29:26Z

I'm not sure if it will break on *nix for this specific case. Can you give it a try?

I will, sure

Also, if we plan to go with .gitattributes, don't we have to include lineEndings 'GIT_ATTRIBUTES' as mentioned here: https://github.com/diffplug/spotless/blob/9aefb65262685a9959360470b070e2e649f038eb/README.md#custom-rules.

Nope, it is a default for a long time: diffplug/spotless#23

reta · 2021-12-01T20:57:37Z

@owaiskazi19 expectation matches, have a branch for you, https://github.com/reta/OpenSearch/tree/issue-1637.apply, what I did

On Windows:

./gradlew.bat :benchmarks:spotlessApply
./gradlew.bat :benchmarks:spotlessJavaCheck - SUCCESS
commit & push

On Linux:

pull
./gradlew.bat :benchmarks:spotlessJavaCheck - FAILURE

> Configure project :qa:os                                                
Cannot add task 'destructiveDistroTest.docker' as a task with that name already exists.       
=======================================                                                                                                                                                      
OpenSearch Build Hamster says Hello!                                                          
  Gradle Version        : 7.3                                                                 
  OS Info               : Linux 5.13.0-22-generic (amd64)                                     
  JDK Version           : 11 (Ubuntu JDK)                                                     
  JAVA_HOME             : /usr/lib/jvm/java-11-openjdk-amd64                 
  Random Testing Seed   : BF2E36EEA16B5B41                                                    
  In FIPS 140 mode      : false                                                               
=======================================                                                       
                                                                                              
> Task :benchmarks:spotlessJavaCheck FAILED                                                   
                                                                                              
FAILURE: Build failed with an exception.

Commit in question: reta@d4e6619

…stems Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta · 2021-12-01T21:01:17Z

.gitattributes

@@ -0,0 +1,2 @@
+* text eol=lf
+*.bat text eol=crlf


Keeping bat files Windows compatible is probably good idea

opensearch-ci-bot · 2021-12-01T21:04:58Z

✅ Gradle Wrapper Validation success 37fad89

opensearch-ci-bot · 2021-12-01T21:13:04Z

✅ Gradle Precommit success 37fad89

opensearch-ci-bot · 2021-12-01T21:42:13Z

✅ Gradle Check success 37fad89
Log 1290

Reports 1290

owaiskazi19

Based on diffplug/spotless#39. This LGTM.

owaiskazi19 · 2021-12-03T20:37:21Z

On Linux:
pull
./gradlew.bat :benchmarks:spotlessJavaCheck - FAILURE`

I am still not sure why are we running gradlew.bat on Linux and not simply ./gradlew. As clearly .bat is for Windows.

The way I was thinking for #1637 is to run SpotlessApply and push those changes. That'll have a successful SpotlessJavaCheck.

To make sure the build is fine, run:

Windows:
./gradlew.bat :benchmarks:spotlessJavaCheck

Linux:
./gradlew :benchmarks:spotlessJavaCheck.

We won't need .gitattributes file this way. Let me know if I missed anything here.

reta · 2021-12-03T21:02:23Z

I am still not sure why are we running gradlew.bat on Linux and not simply ./gradlew. As clearly .bat is for Windows.

@owaiskazi19 ./gradlew.bat is obviously a mistake (I copy/pasted the details from 2 different boxes), for Linux, should be ./gradlew, the outcome is the same.

The way I was thinking for #1637 is to run SpotlessApply and push those changes. That'll have a successful SpotlessJavaCheck.

Please go ahead, as I explained to, I did that and I have a branch with those changes pushed, https://github.com/reta/OpenSearch/tree/issue-1637.apply, you are welcome to pull it and run ./gradlew :benchmarks:spotlessJavaCheck on Linux, unless you have Git configuration in place to manipulate EOL (see please https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings), the build fails.

dblock · 2021-12-03T21:02:41Z

The way I was thinking for #1637 is to run SpotlessApply and push those changes. That'll have a successful SpotlessJavaCheck.

Will it or will it start breaking on *nix? If yes, let's do that instead of modifying .gitattributes? I am honestly not sure what "the right thing to do" is.

reta · 2021-12-03T21:04:16Z

Will it or will it start breaking on *nix? If yes, let's do that instead of modifying .gitattributes? I am honestly not sure what "the right thing to do" is.

@dblock it fails for me on *nix, passes on Windows.

dblock · 2021-12-03T21:05:28Z

Will it or will it start breaking on *nix? If yes, let's do that instead of modifying .gitattributes? I am honestly not sure what "the right thing to do" is.

@dblock it fails for me on *nix, passes on Windows.

That's what I thought. Spotless needs to be told to be LF on Windows, which is done in this PR. I'm going to merge this.

dblock · 2021-12-03T21:06:45Z

@owaiskazi19 Did I miss anything? Merge?

reta · 2021-12-03T21:08:28Z

@dblock fyi, just to point out a few references, all related to spotless:

owaiskazi19 · 2021-12-03T21:11:18Z

@owaiskazi19 Did I miss anything? Merge?

Since Spotless copies git's behavior. This change will set Spotless to use LF and that's it.
I don't see any issue in merging it. Let's see what it open for us in the future.
Thanks @reta for the efforts and getting this change in!

reta · 2021-12-03T21:14:52Z

Thanks @owaiskazi19 !

reta · 2021-12-03T21:22:10Z

@dblock sorry, I am a bit confused by "backport" process, is it automated or still manual? (I don't see backport pull request to 1.x)

VachaShah · 2021-12-03T21:50:24Z

Hi @reta, it is automated but the label backport 1.x has to be added before the PR is merged and the workflow should run as part of the checks on the PR which doesn't look to be the case with this PR. I am not sure if there is a way to re-run the checks for a merged PR.

This is an example of how it should run VachaShah/TestGithubActions#6 (its a test repo I have) here I added the backport release-branch label and the Backport workflow ran as a check on the PR. Same thing should happen on this PR for it to detect that a backport is requested.

reta · 2021-12-03T21:54:56Z

@VachaShah ah ... this is what I suspected, thank you, AFAIK external contributors do not have permissions to add labels (at least, that was certainly true before not long ago). I will try to add one next time, thank you!

…stems (opensearch-project#1638) Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

dblock · 2021-12-06T13:07:22Z

Hi @reta, it is automated but the label backport 1.x has to be added before the PR is merged

Oh! I didn't know that. So if I see a PR that I want backported, I can't just label it and then merge? What should I do in that case?

…stems (#1638) (#1655) Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

VachaShah · 2021-12-08T05:57:43Z

Hi @reta, it is automated but the label backport 1.x has to be added before the PR is merged

Oh! I didn't know that. So if I see a PR that I want backported, I can't just label it and then merge? What should I do in that case?

Yes you can add the label before the merge and wait for the workflow to run. The workflow will start running as soon as the label is added and finish in some seconds (I tried this on one of the PRs). Once it finishes, the PR can be merged and a backport PR will be opened automatically. For the previous PRs, the workflow was not running as the Github action was not added in the list that is allowed to run. It works now successfully.

reta requested a review from a team as a code owner December 1, 2021 17:43

saratvemulapalli requested a review from owaiskazi19 December 1, 2021 17:45

reta force-pushed the issue-1637 branch from 6e078d3 to 82f707c Compare December 1, 2021 18:17

reta force-pushed the issue-1637 branch from 82f707c to ebc8654 Compare December 1, 2021 19:05

reta force-pushed the issue-1637 branch from ebc8654 to b581e54 Compare December 1, 2021 19:48

reta force-pushed the issue-1637 branch from b581e54 to ebf5367 Compare December 1, 2021 20:24

Added .gitattributes to manage end-of-line checks for Windows/*nix sy…

37fad89

…stems Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta force-pushed the issue-1637 branch from ebf5367 to 37fad89 Compare December 1, 2021 21:00

reta commented Dec 1, 2021

View reviewed changes

owaiskazi19 approved these changes Dec 3, 2021

View reviewed changes

dblock approved these changes Dec 3, 2021

View reviewed changes

dblock merged commit 70f0787 into opensearch-project:main Dec 3, 2021

dblock added backport 1.x pending backport Identifies an issue or PR that still needs to be backported labels Dec 3, 2021

reta mentioned this pull request Dec 3, 2021

[Backport] [1.x] Added .gitattributes to manage end-of-line checks for Windows/*nix systems (#1638) #1655

Merged

5 tasks

reta added a commit to reta/OpenSearch that referenced this pull request Dec 4, 2021

Added .gitattributes to manage end-of-line checks for Windows/*nix sy…

87c9ab3

…stems (opensearch-project#1638) Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

dblock pushed a commit that referenced this pull request Dec 6, 2021

Added .gitattributes to manage end-of-line checks for Windows/*nix sy…

92e0831

…stems (#1638) (#1655) Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

Added .gitattributes to manage end-of-line checks for Windows/*nix systems #1638

Added .gitattributes to manage end-of-line checks for Windows/*nix systems #1638

Conversation

reta commented Dec 1, 2021 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

owaiskazi19 commented Dec 1, 2021 • edited Loading

opensearch-ci-bot commented Dec 1, 2021

dblock commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

reta commented Dec 1, 2021 • edited Loading

owaiskazi19 commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

reta commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

owaiskazi19 commented Dec 1, 2021 • edited Loading

reta commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

owaiskazi19 commented Dec 1, 2021 • edited Loading

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

reta commented Dec 1, 2021 • edited Loading

reta commented Dec 1, 2021 • edited Loading

reta Dec 1, 2021

Choose a reason for hiding this comment

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

opensearch-ci-bot commented Dec 1, 2021

owaiskazi19 left a comment

Choose a reason for hiding this comment

owaiskazi19 commented Dec 3, 2021 • edited Loading

reta commented Dec 3, 2021 • edited Loading

dblock commented Dec 3, 2021 • edited Loading

reta commented Dec 3, 2021 • edited Loading

dblock commented Dec 3, 2021

dblock commented Dec 3, 2021

reta commented Dec 3, 2021

owaiskazi19 commented Dec 3, 2021 • edited Loading

reta commented Dec 3, 2021

reta commented Dec 3, 2021

VachaShah commented Dec 3, 2021

reta commented Dec 3, 2021

dblock commented Dec 6, 2021

VachaShah commented Dec 8, 2021

reta commented Dec 1, 2021 •

edited

Loading

owaiskazi19 commented Dec 1, 2021 •

edited

Loading

reta commented Dec 1, 2021 •

edited

Loading

owaiskazi19 commented Dec 1, 2021 •

edited

Loading

owaiskazi19 commented Dec 1, 2021 •

edited

Loading

reta commented Dec 1, 2021 •

edited

Loading

reta commented Dec 1, 2021 •

edited

Loading

owaiskazi19 commented Dec 3, 2021 •

edited

Loading

reta commented Dec 3, 2021 •

edited

Loading

dblock commented Dec 3, 2021 •

edited

Loading

reta commented Dec 3, 2021 •

edited

Loading

owaiskazi19 commented Dec 3, 2021 •

edited

Loading