CORE-20557: Fix issue in CPB and CPK plugins preventing dependencies of transitive dependencies from being included #604

malachyb · 2024-06-24T14:13:22Z

The packaging plugins had an issue which prevented certain transitive dependencies from being included in the outputs, which meant that packages had to declare transitive dependencies themself. This PR changes that by creating a temporary directory where jar files are extracted from the CPB file of declared dependencies and including those files as dependencies.

…of transitive dependencies from being included

Sakpal · 2024-06-26T13:14:15Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

+            FileCollection jars = getInputs().getFiles();
+            Set<String> jarNames = new HashSet<>();
+            for (File file : jars) {
+                jarNames.add(file.getName());


Can we move this inside the next for loop, which is iterating through the same FileCollection?

Sakpal · 2024-06-26T13:17:27Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbPlugin.java

@@ -73,6 +73,7 @@ public void apply(@NotNull Project project) {
            cpbTask.getArchiveVersion().convention(cpkTask.flatMap(Jar::getArchiveVersion));

            cpbTask.doFirst(task -> cpbTask.checkForDuplicateCpkCordappNames());
+            cpbTask.doFirst(task -> cpbTask.extractTransitiveDeps());


How much time has this task added to the entire build process?

Running a clean build on the DC repo (which creates 11 cpbs) 3 times on the previous version took me an average of 38.396 seconds. That same build with the new version 3 times took an average of 38.244 seconds. Since I doubt I've sped up the process I'm interpreting that as my changes not having much of an effect on the build time.

kyriathar

The current review (comments) is just epidermic on minor things as a first pass.

A few questions on the description:

The packaging plugins had an issue which prevented certain transitive dependencies from being included in the outputs, which meant that packages had to declare transitive dependencies themself.

What do we mean with "certain"? Isn't this issue happening for all transitive dependencies?

This PR changes that by creating a temporary directory where jar files are extracted from the CPB file of declared dependencies and including those files as dependencies.

I think we already talked about this so just confirming; eventually we are emitting the transitive dependencies in the flat list of dependencies we have in CPB right?

kyriathar · 2024-07-09T14:47:21Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpk2/DependencyCalculator.java

+            Files.copy(inputStream, path);
+            path.toFile().deleteOnExit();
+        }
+        file = path.toFile();


Let's put together definition of var and initialization

kyriathar · 2024-07-09T15:14:38Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpk2/DependencyCalculator.java

+    }
+
+    @NotNull
+    private static Set<File> extractJarsFromCpb(Set<File> cpbs, Set<String> cordappFileNames) throws IOException {


This function could be named extractTransitiveDependeciesFromCpb?

kyriathar · 2024-07-09T15:19:31Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpk2/DependencyCalculator.java

+    @NotNull
+    private static Set<File> extractJarsFromCpb(Set<File> cpbs, Set<String> cordappFileNames) throws IOException {
+        Path pathToDir = Files.createTempDirectory("jars");
+        pathToDir.toFile().deleteOnExit();


Why deleteOnExit and not handle it ourselves synchronously?

I thought using the built-in deleteOnExit made more sense than making our own implementation since the files will be used in a separate class from where we create them and passing the data between them would make things more complicated.

kyriathar · 2024-07-09T15:19:50Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpk2/DependencyCalculator.java

+
+    @NotNull
+    private static Set<File> extractJarsFromCpb(Set<File> cpbs, Set<String> cordappFileNames) throws IOException {
+        Path pathToDir = Files.createTempDirectory("jars");


Maybe rename to tempDir?

kyriathar · 2024-07-09T15:21:49Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

+            jarDir.deleteOnExit();
+        } catch (IOException e) {
+            getLogger().warn("Could not create jar directory: {}", e.getMessage());
+            jarDir = null;


This probably is still null in this case

kyriathar · 2024-07-09T15:23:33Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

    }

    @Override
    @NotNull
    public AbstractCopyTask from(@NotNull Object... args) {
+        args = Arrays.copyOf(args, args.length + 1);
+        args[args.length - 1] = jarDir;


Where are these arguments passed to?

The arguments are passed to super.from below which is used to source the files that will be included in the zip file

kyriathar · 2024-07-09T15:26:44Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

@@ -104,4 +123,55 @@ public void checkForDuplicateCpkCordappNames() {
            }
        }
    }
+
+    public void extractTransitiveDeps() {
+        if (jarDir != null) {


Would we rather want to checkNotNull here? I think we always expect this dir to exist?

kyriathar · 2024-07-09T15:30:38Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

+            FileCollection jars = getInputs().getFiles();
+            Set<String> jarNames = new HashSet<>();
+            Set<File> cpbs = new HashSet<>();
+            for (File file : jars) {


kyriathar · 2024-07-09T15:31:37Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

+            Set<File> cpbs = new HashSet<>();
+            for (File file : jars) {
+                jarNames.add(file.getName());
+                File parent = file.getParentFile();


What kind of file is this that we expect it to have multiple CPBs?

Good point, I don't think that's needed.

josephzunigadaly · 2024-07-15T11:50:08Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

+        } catch (IOException e) {
+            getLogger().warn("Could not create jar directory: {}", e.getMessage());
+            jarDir = null;
+        }


I think we should re-throw the exception or remove the try/catch because this is needed for the bug fix to work. If we drop the exception and continue, we are allowing the incorrect behaviour to continue happening in this error path.

It is better to stop the build with an error. The end user will see the build fail and can fix the cause of the error. If we continue, we will build outputs with missing dependencies and gradle will print out "success" at the end of the build.

I've changed it so it fails with the error now

malachyb · 2024-07-15T15:36:05Z

@kyriathar

What do we mean with "certain"? Isn't this issue happening for all transitive dependencies?

I probably didn't word that as well as I could have. It's under specific circumstances that the issue occurs i.e. when we import a project dependency that has its own transitive dependencies.

I think we already talked about this so just confirming; eventually we are emitting the transitive dependencies in the flat list of dependencies we have in CPB right?

Yes, we place the jars in the temporary directory into the flat list of dependencies.

CORE-20557: Fix issue in CPB and CPK plugins preventing dependencies …

6d1a29b

…of transitive dependencies from being included

malachyb force-pushed the malachy/CORE-20557 branch from 1acdbf7 to 6d1a29b Compare June 24, 2024 14:14

malachyb added 2 commits June 24, 2024 15:21

Add NotNull to DependencyCalculator::extractJarEntry

27402c4

Add tests for CPK and CPB functionality

3164b58

malachyb marked this pull request as ready for review June 25, 2024 10:16

Sakpal reviewed Jun 26, 2024

View reviewed changes

Remove double for loop

02671e4

malachyb requested review from kyriathar and josephzunigadaly July 15, 2024 11:19

kyriathar reviewed Jul 15, 2024

View reviewed changes

josephzunigadaly reviewed Jul 15, 2024

View reviewed changes

Correct feedback

a28278c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-20557: Fix issue in CPB and CPK plugins preventing dependencies of transitive dependencies from being included #604

CORE-20557: Fix issue in CPB and CPK plugins preventing dependencies of transitive dependencies from being included #604

malachyb commented Jun 24, 2024

Sakpal Jun 26, 2024

Sakpal Jun 26, 2024

malachyb Jun 26, 2024

kyriathar left a comment

kyriathar Jul 9, 2024

kyriathar Jul 9, 2024

kyriathar Jul 9, 2024

malachyb Jul 15, 2024

kyriathar Jul 9, 2024

kyriathar Jul 9, 2024

kyriathar Jul 9, 2024

malachyb Jul 15, 2024

kyriathar Jul 9, 2024

kyriathar Jul 9, 2024

kyriathar Jul 9, 2024

malachyb Jul 15, 2024

josephzunigadaly Jul 15, 2024 •

edited

Loading

malachyb Jul 15, 2024

malachyb commented Jul 15, 2024

CORE-20557: Fix issue in CPB and CPK plugins preventing dependencies of transitive dependencies from being included #604

Are you sure you want to change the base?

CORE-20557: Fix issue in CPB and CPK plugins preventing dependencies of transitive dependencies from being included #604

Conversation

malachyb commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyriathar 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephzunigadaly Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malachyb commented Jul 15, 2024

josephzunigadaly Jul 15, 2024 •

edited

Loading