This is for NoticeTask for issue #34459 #37032

mike2ykme · 2018-12-29T19:57:06Z

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

elasticmachine · 2018-12-30T01:26:09Z

Pinging @elastic/es-core-infra

alpar-t

Thanks for your contribution @mike2ykme !

I left a few comments.

alpar-t · 2019-01-07T12:58:49Z

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

+ */
+public class NoticeTask extends DefaultTask {
+
+    @InputFile


The annotations have no effect on the private fields, they need to be placed on public getters.

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

alpar-t · 2019-01-07T13:10:36Z

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

+        licensesDirs.add(licensesDir);
+    }
+
+    public void addLicenseDir(File licenseDir){


We don't need another method that does the exact thing.

alpar-t · 2019-01-07T13:10:54Z

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

+        return Collections.unmodifiableList(this.licensesDirs);
+    }
+
+//    public static String getText(File file){


please remove commented code.

alpar-t · 2019-01-07T13:21:48Z

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

+
+    public static void writeToFile(File file, String output, Charset charset){
+
+        try(BufferedWriter out = Files.newBufferedWriter(file.toPath(),charset)){


would prefer to useFiles.write for consistency here.

alpar-t · 2019-01-07T13:22:37Z

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

+public class NoticeTaskTest extends GradleUnitTestCase {
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();


The Gradle test project creates it's own temporary folder no need to pass it in.

alpar-t · 2019-01-07T13:24:28Z

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

+
+    public List<File> getListWithoutCopies() throws IOException{
+        List<File> files = this.getListWithCopies();
+        files.remove(2);


This makes the test hard to read, I would rather create the list without copies here and add to it above.

alpar-t · 2019-01-07T13:31:46Z

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

+
+
+        try {
+            noticeTask.generateNotice();


This won't fail if an exception is not thrown as it should.
Use:

@Rule public ExpectedException expectedEx = ExpectedException.none();

instead.

I updated to now expect a RuntimeException with a certain a certain error message.

I think you should use the ExpectedException rule.
I don't think this test passes as it's written now.
The correct way would be to have a fail() as the last line in try, but the rule reads better.

alpar-t · 2019-01-07T13:34:21Z

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

+    }
+
+    private NoticeTask createTask(Project project) {
+        return (NoticeTask) project.task(singletonMap("type", NoticeTask.class), "NoticeTask");


The cast can be avoided by using project.getTasks().create

…032/files

removed commented out lines

alpar-t

Thanks for the updates, I left some more comments.

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

alpar-t · 2019-01-08T10:33:07Z

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

+        final StringBuilder output = new StringBuilder();
+
+
+        try (Stream<String> lines = Files.lines(this.inputFile.toPath(), StandardCharsets.UTF_8)) {


please use a single line for spacing

alpar-t · 2019-01-08T10:33:40Z

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

+        licensesDirs.forEach(file -> {
+            try (Stream<Path> pathStream = Files.walk(file.toPath())) {
+                pathStream
+//                    .filter(path -> path.endsWith("-NOTICE.txt"))


no commented code please

alpar-t · 2019-01-08T10:34:16Z

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

+                            seen.put(name, licenseFile);
+                        }
+                    });
+            } catch (IOException e) {


No need to catch this here.

These are caught as checked and re-thrown as unchecked because they are being handled inside a lambda expression inside a forEach() on the list

alpar-t · 2019-01-08T10:44:26Z

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

+                builder.append(buffer, 0, read);
+            }
+
+        } catch (IOException e) {


It's generally not desirable to catch IOException low level, these should be bubbled up.

alpar-t · 2019-01-08T10:46:25Z

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

+
+
+        try {
+            noticeTask.generateNotice();


I think you should use the ExpectedException rule.
I don't think this test passes as it's written now.
The correct way would be to have a fail() as the last line in try, but the rule reads better.

Removed TemporaryFolder and replaced with Gradle temporaryDir removed commented out code replaced getText with Apache FileUtils.readFileToString()

rjernst · 2019-01-08T18:56:45Z

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

+        // party component names, unaffected by the full path to the various files
+        final Map<String, File> seen = new TreeMap<>();
+
+        licensesDirs.forEach(file -> {


Is a forEach really necessary? A straight for loop would be just as readable, and would not require catching/rethrowing unchecked IOException

Oh yeah that’s a good point. I’ll make the change.

pulled loop/stream to get notices out of given directories into separate function to allow throwing of IOException

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

alpar-t · 2019-01-10T16:00:42Z

@elasticmachine test this please

alpar-t · 2019-01-14T14:33:59Z

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

+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.*;


we don't allow * imports, this is why the build fails.

This has been replaced with individual imports.

alpar-t · 2019-01-14T14:36:31Z

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

+import java.util.ArrayList;
+import java.util.List;
+
+import static org.apache.commons.io.FileUtils.write;


commons io is only available via a transitive dependency in buildSrc, it's not something we should be using, let's just stick to Files

When I tried to assemble the project with just the Files functions I ran into issues where Charsets were not matching. IE UTF-8 wouldn't always be used. I've changed the function to simply ignore bad characters using the Java IO/NIO functions.

alpar-t · 2019-01-14T14:39:11Z

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

+            appendFileToOutput(licenseFile, name, "LICENSE", output);
+        }
+
+        Files.write(outputFile.toPath(), output.toString().getBytes());


should specify a char set here.

Reading the files with Files.lines and such with just a Charset would fail if the file being read was not of the expected file format. I've now made it simply ignore the character to avoid any problems with the file being written back as UTF-8

mike2ykme · 2019-01-16T04:03:45Z

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

+            }
+        }
+        return licensesSeen;
+    }


I ran into issues with using the NIO Files methods using the UTF-8 charsets failing when they were reading the notice files and would run into a "java.nio.charset.MalformedInputException: Input length = 1" when running assemble command using Gradle. So I used the decoder here to cause it to replace the bad characters with the expected bad characters for UTF-8 so it can be clearly seen what characters were replaced.

We prefer not to allow leniency in general, so it would be preferable to throw a meaningful error showing the file(s) that don't have the correct encoding and then fix those up rather than live with inconsistent encodings or broken characters.

@elasticmachine test this please

danielmitterdorfer · 2019-02-20T12:37:42Z

@mike2ykme are you still intersted in getting this in?

mike2ykme · 2019-02-20T14:00:47Z

Yes, is there anything I need to do for this? I thought it was just waiting on being tested.

alpar-t · 2019-02-26T09:22:58Z

@mike2ykme I had a comment w.r.t. the bad encoding, and throwing an error and fixing them rather than ignoring it above.
Note that you'll also have to merge master in to get the checks passing.

… fail cause any issues The NoticeTask will now throw a RuntimeException indicating which file caused the problem. The MalformedInputException is wrapped inside a java.io.UncheckedException by Files.lines(),

mike2ykme · 2019-02-27T05:34:18Z

Thanks! I have now removed the code that would just ignore the bad character sets. Now it will throw an exception indicating there is a problem. I have noticed that Files.lines() wraps the MalformedInputException in a java.io.UncheckedException so I have caught these and rethrown a RuntimeException with the purpose of showing which file is causing the problem to avoid having to guess at where the problem is occuring at.

alpar-t · 2019-02-28T07:52:53Z

thanks @mike2ykme !
I suspect we'll now have a failing build that will highlight the bad engodings.
We won't be able to merge this RP if it fails the build, so we'll also have to fix those files in here.

@elasticmachine test this please

javanna · 2019-07-15T09:55:49Z

Do we plan on merging this or shall we close it?

alpar-t · 2019-07-16T05:36:48Z

I'm going to close it @mike2ykme feel free to reopen if you plan to work on this again

mike2ykme added 2 commits December 29, 2018 13:42

Commit to add NoticeTask to Java source

4719951

Commit to add NoticeTask to Java source

21da41e

dnhatn added the :Delivery/Build Build or test infrastructure label Dec 30, 2018

wuyazhan approved these changes Dec 30, 2018

View reviewed changes

alpar-t reviewed Jan 7, 2019

View reviewed changes

mike2ykme added 3 commits January 7, 2019 18:09

Updated per notes on https://github.com/elastic/elasticsearch/pull/37…

81ef89a

…032/files

Updated per notes on https://github.com/elastic/elasticsearch/pull/37…

ae7f102

…032/files

Updated per: https://github.com/elastic/elasticsearch/pull/37032/files

9de4048

removed commented out lines

alpar-t reviewed Jan 8, 2019

View reviewed changes

Updated per: https://github.com/elastic/elasticsearch/pull/37032/files

1eee007

Removed TemporaryFolder and replaced with Gradle temporaryDir removed commented out code replaced getText with Apache FileUtils.readFileToString()

rjernst reviewed Jan 8, 2019

View reviewed changes

mike2ykme added 2 commits January 9, 2019 11:57

Updated per: https://github.com/elastic/elasticsearch/pull/37032/files

ec5c5e2

pulled loop/stream to get notices out of given directories into separate function to allow throwing of IOException

fixed typo in name of getFilesToAppend() method

e58e252

mike2ykme commented Jan 9, 2019

View reviewed changes

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

mike2ykme commented Jan 9, 2019

View reviewed changes

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

alpar-t reviewed Jan 14, 2019

View reviewed changes

mike2ykme commented Jan 16, 2019

View reviewed changes

danielmitterdorfer added the feedback_needed label Feb 20, 2019

Renamed tests class to NoticeTaskTests with an (s) to ensure it won't…

d672af5

… fail cause any issues The NoticeTask will now throw a RuntimeException indicating which file caused the problem. The MalformedInputException is wrapped inside a java.io.UncheckedException by Files.lines(),

javanna removed the feedback_needed label Jul 15, 2019

alpar-t closed this Jul 16, 2019

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


		public static void writeToFile(File file, String output, Charset charset){

		try(BufferedWriter out = Files.newBufferedWriter(file.toPath(),charset)){

		final StringBuilder output = new StringBuilder();


		try (Stream<String> lines = Files.lines(this.inputFile.toPath(), StandardCharsets.UTF_8)) {

This is for NoticeTask for issue #34459 #37032

This is for NoticeTask for issue #34459 #37032

Conversation

mike2ykme commented Dec 29, 2018

elasticmachine commented Dec 30, 2018

alpar-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmitterdorfer commented Feb 20, 2019

mike2ykme commented Feb 20, 2019

alpar-t commented Feb 26, 2019

mike2ykme commented Feb 27, 2019

alpar-t commented Feb 28, 2019

javanna commented Jul 15, 2019

alpar-t commented Jul 16, 2019