Implement ConcatFilesTask from Groovy to Java issues#34459 by akki · Pull Request #37497 · elastic/elasticsearch

akki · 2019-01-15T18:07:04Z

Issue #34459

Facing one problem (added as comment), after which the implementation would be complete for review from my side.

akki · 2019-01-15T18:22:41Z

If this looks like a good enough implementation, I can start writing any tests that are required.

(And sorry for the force pushes, I forgot those are not allowed even in forks in Elasticsearch)

rjernst

I have some requests

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java

elasticmachine · 2019-01-16T12:09:48Z

Pinging @elastic/es-core-infra

akki · 2019-01-20T19:33:40Z

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java

+
+        // To remove duplicate lines
+        LinkedHashSet<String> uniqueLines = new LinkedHashSet<>();
+        for (File f : getFiles()) {


I am not sure if looping over a FileTree instance this way is correct.

akki · 2019-01-20T19:38:04Z

@elasticmachine Test this please.

@rjernst Thanks for the review, I have incorporated your suggestions to the best of my understanding.

I ran ./gradlew check locally this time around 2 hours ago and it is still running. Is this the expected time it takes and is there any way to make it run faster?

alpar-t · 2019-01-21T10:39:42Z

@elasticmachine test this please
@akki you can do ./gradlew -p distribution check for this particular change. A full check does take more than 2 hours to run on powerful desktop machine. There's no way to make it faster, we're working on it.

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java

alpar-t · 2019-01-21T10:41:58Z

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java

+
+    @TaskAction
+    public void concatFiles() throws IOException {
+        final String encoding = "UTF-8";


Just use StandardCharsets.UTF_8, no need for a variable.

@atorok
There were multiple places where I had to mention the encoding type (UTF-8), hence I used it as a variable to keep it DRY.
Please let me know if it still makes more sense to you to remove the variable.

I do think it's better to use the StandardCharsets.UTF_8 constant to keep it dry rather than our own variable.

Ahh, now I see your point. Fixed it, thanks! :)

alpar-t

Please also write tests for the task.

akki · 2019-01-25T17:24:56Z

@atorok I have added the setter for target. Can you please help me test this.

I tried running ./gradlew -p distribution check and it worked well for me. But recently it started giving me the following errors (maybe because I tried running gradlew idea but I am not sure about that)

1: Task failed with an exception.
-----------
* Where:
Build file '/Users/adoshi/Work/repos/elasticsearch/benchmarks/build.gradle' line: 20

* What went wrong:
A problem occurred evaluating project ':benchmarks'.
> Failed to apply plugin [id 'elasticsearch.build']
   > A problem occurred starting process 'command '/bin/jrunscript''

2: Task failed with an exception.
-----------
* What went wrong:
A problem occurred configuring project ':benchmarks'.
> Must specify license and notice file for project :benchmarks

The same error is thrown when I run time ./gradlew test -Dtests.class="org.elasticsearch.gradle.ConcatFilesTaskTests" --scan. Do you clue why this might be happening and what I am doing wrong?

akki · 2019-01-28T03:34:01Z

Fixed the above issue - seemed like a problem with the Java version and environment variables.
Having some problem with writing the tests (Java newbie here 😬) so might take some more time.

akki · 2019-01-29T17:00:55Z

buildSrc/src/test/java/org/elasticsearch/gradle/ConcatFilesTaskTests.java

+        file.getParentFile().mkdirs();
+        file.createNewFile();
+        concatFilesTask.setTarget(file);
+        concatFilesTask.setFiles(new FileTree());


This fails saying FileTree is abstract; cannot be instantiated.

That's right, you'll have to use the class as intended by Gradle projcet.fileTree in this case.
See: https://docs.gradle.org/current/javadoc/org/gradle/api/file/FileTree.html

akki · 2019-01-29T17:04:43Z

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java

-        for (File f : getFiles()) {
-            uniqueLines.addAll(Files.readAllLines(f.toPath(), Charset.forName(encoding)));
-        }
+        getFiles().getFiles().forEach(f -> uniqueLines.addAll(fileReadAllLines(f, encoding)));


This fails with NullPointerException when setFiles is not used in the tests (I guess because the first getFiles() returns null?).

akki · 2019-01-29T17:07:55Z

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java

-        for (File f : getFiles()) {
-            uniqueLines.addAll(Files.readAllLines(f.toPath(), Charset.forName(encoding)));
-        }
+        getFiles().getFiles().forEach(f -> uniqueLines.addAll(fileReadAllLines(f, encoding)));


Also, here Files.readAllLines raises the error unreported exception IOException; must be caught or declared to be thrown, even though the definition of concatFilesTask says throws IOException. Hence the need for creating an Exception-safe function fileReadAllLines.

akki · 2019-01-29T17:09:38Z

Hi @atorok I tried to write the tests for this task but facing some problems using FileTree - I couldn't find any other test case similar to this task.
I would appreciate a lot if you could point me to any similar test cases or help me in any other way to write these. You can find my attempt in this commit (I've added a few comments to explain my problem).

akki · 2019-02-07T04:38:52Z

Just wanted to check in once again on this PR to request if somebody has any suggestions for me on how I can go ahead with writing the test cases.
If not, is it possible for the admins to merge this PR in the current state?

alpar-t · 2019-02-07T10:18:17Z

Sorry to keep you waiting @akki I will look at this early next week.

akki · 2019-02-07T16:44:03Z

I understand, appreciate the update on your availability. :)

danielmitterdorfer · 2019-02-13T09:34:48Z

Note to reviewers: This PR will make #34476 obsolete. Please close #34476 accordingly when merging this PR.

alpar-t · 2019-02-13T14:09:01Z

@akki see here for file tree: https://docs.gradle.org/current/javadoc/org/gradle/api/file/FileTree.html you'll have to use the class as intended by Gradle. Specifically use project.fileTree

akki · 2019-02-20T03:12:30Z

Thanks for the hint; I'll get back to this by next week.

akki · 2019-03-02T16:03:33Z

Hi @atorok / all
I have added the tests as requested. Please have a look.
I did not run the whole test suite as it takes forever to run on my machine. I did run ./gradlew test -Dtests.class=org.elasticsearch.gradle.ConcatFilesTaskTests and ./gradlew -p distribution check - both of them passed for me.

Please let me know your thoughts.

akki · 2019-03-02T16:04:13Z

@elasticmachine test this please

alpar-t · 2019-03-04T05:26:38Z

@elasticmachine test this please

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java

buildSrc/src/test/java/org/elasticsearch/gradle/ConcatFilesTaskTests.java

* Use standardcharset everywhere * Reshuffling of imports * Cosmetic changes

akki · 2019-03-04T17:36:16Z

@atorok Thank you for the review. I have updated the PR to address your comments.
I also had a look at the build failures which seem to pointing to some problem with "git branch checkouts". Do you believe these are related to this PR and code changes? I digged in a bit but couldn't find any connection between the failing task and ConcatFilesTask.

…oovy

alpar-t · 2019-03-06T14:10:04Z

@akki those failures are indeed not related to the changes, they are happening because the PR was created a long time ago and master got out of sync. I merged master in so @elasticmachine test this please.

akki · 2019-03-06T15:45:08Z

@atorok Ahh, thank you. I did not realise that because GitHub told me This branch has no conflicts with the base branch.

relates #344597

akki · 2019-03-07T13:37:27Z

@atorok Thanks for the approval. You might also want to close #34476 now as it is obsolete.

akki force-pushed the 34459-concatFilesGroovy branch from 695e6e7 to 800bdb3 Compare January 15, 2019 18:15

Implement ConcatFilesTask from Groovy to Java issues#34459

af7f347

akki force-pushed the 34459-concatFilesGroovy branch from 800bdb3 to af7f347 Compare January 15, 2019 18:16

akki changed the title ~~WIP: Implement ConcatFilesTask from Groovy to Java issues#34459~~ Implement ConcatFilesTask from Groovy to Java issues#34459 Jan 15, 2019

rjernst reviewed Jan 15, 2019

View reviewed changes

colings86 added the :Delivery/Build Build or test infrastructure label Jan 16, 2019

Update code according to review

8909525

akki commented Jan 20, 2019

View reviewed changes

alpar-t reviewed Jan 21, 2019

View reviewed changes

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java Outdated Show resolved Hide resolved

alpar-t reviewed Jan 21, 2019

View reviewed changes

alpar-t requested changes Jan 21, 2019

View reviewed changes

Add setter function for target

f763b45

akki added 2 commits January 29, 2019 23:29

WIP: Attempt to writing tests for ConcatFilesTask

765c66f

Use StandardCharsets constant rather than own variable

231b119

akki commented Jan 29, 2019

View reviewed changes

danielmitterdorfer mentioned this pull request Feb 13, 2019

convert ConcatFilesTask.groovy to .java #34476

Closed

Complete tests and final working code

a4f9131

Remove unused imports from ConcatFilesTaskTests

60c1202

alpar-t reviewed Mar 4, 2019

View reviewed changes

buildSrc/src/main/java/org/elasticsearch/gradle/ConcatFilesTask.java Outdated Show resolved Hide resolved

alpar-t reviewed Mar 4, 2019

View reviewed changes

buildSrc/src/test/java/org/elasticsearch/gradle/ConcatFilesTaskTests.java Outdated Show resolved Hide resolved

alpar-t reviewed Mar 4, 2019

View reviewed changes

buildSrc/src/test/java/org/elasticsearch/gradle/ConcatFilesTaskTests.java Outdated Show resolved Hide resolved

Changes according to review and refactor

1b67905

* Use standardcharset everywhere * Reshuffling of imports * Cosmetic changes

alpar-t added 2 commits March 6, 2019 16:06

Merge remote-tracking branch 'origin/master' into 34459-concatFilesGr…

66feb00

…oovy

fix checkstyle

3bb3ee3

alpar-t approved these changes Mar 7, 2019

View reviewed changes

alpar-t added v8.0.0 v7.2.0 >non-issue labels Mar 7, 2019

alpar-t merged commit 8657e6e into elastic:master Mar 7, 2019

alpar-t pushed a commit that referenced this pull request Mar 7, 2019

Implement ConcatFilesTask from Groovy to Java issues#34459 (#37497)

4df26a2

relates #344597

akki deleted the 34459-concatFilesGroovy branch July 2, 2019 15:45

alpar-t mentioned this pull request Jul 3, 2019

Replace groovy build-tools (buildSrc) with java #34459

Open

31 tasks

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

akki commented Jan 15, 2019

Uh oh!

akki commented Jan 15, 2019

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Jan 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akki commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alpar-t commented Jan 21, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t left a comment

Choose a reason for hiding this comment

Uh oh!

akki commented Jan 25, 2019

Uh oh!

akki commented Jan 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akki Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akki commented Jan 29, 2019

Uh oh!

akki commented Feb 7, 2019

Uh oh!

alpar-t commented Feb 7, 2019

Uh oh!

akki commented Feb 7, 2019

Uh oh!

danielmitterdorfer commented Feb 13, 2019

Uh oh!

alpar-t commented Feb 13, 2019

Uh oh!

akki commented Feb 20, 2019

Uh oh!

akki commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akki commented Mar 2, 2019

Uh oh!

alpar-t commented Mar 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akki commented Mar 4, 2019

Uh oh!

alpar-t commented Mar 6, 2019

Uh oh!

akki commented Mar 6, 2019

akki commented Jan 20, 2019 •

edited

Loading

akki Jan 29, 2019 •

edited

Loading

akki commented Mar 2, 2019 •

edited

Loading