Migrate from core native to Nokee plugins #298

lacasseio · 2021-11-27T18:55:27Z

Fixes #297.

See the attached issues for general work items left to do. Feel free to provide any kind of feedback on this PR either by starting a new conversation or contributing to a current conversation.

lacasseio · 2021-11-27T18:57:34Z

@big-guy At the moment, the branch is using a local version so the CI will fail. I will cut a release for you once our CI stabilizes.

lacasseio · 2021-11-27T18:58:23Z

buildSrc/build.gradle

@@ -30,6 +32,10 @@ gradlePlugin {
            id = "gradlebuild.jni"
            implementationClass = "gradlebuild.JniPlugin"
        }
+        jniNokee {
+            id = "gradlebuild.jni-nokee"


💄 This is a temporary plugin that will replace the original gradlebuild.jni before merging.

lacasseio · 2021-11-27T19:03:06Z

buildSrc/src/main/java/gradlebuild/WindowsDistribution.java

+
+import org.gradle.api.Named;
+
+public enum WindowsDistribution implements Named {


🔍 The WindowsDistribution refers here to the WINDOWS_MIN which is in fact a way to differentiate between XP and lower vs Vista and higher.

lacasseio · 2021-11-27T19:06:16Z

buildSrc/src/main/java/gradlebuild/WindowsDistribution.java

+
+    @Override
+    public String getName() {
+        return name();


❓ We will need to figure out the correct name to use here. Nokee uses the name of the dimension value to generate names (tasks, variants, etc.). We could use min and empty string to ensure the names aligns with the software model plugins but the empty string can be problematic as it can cause domain object collision. At the moment Nokee doesn't disallow it which is an open question and definitely not a defined behaviour.

lacasseio · 2021-11-27T19:09:24Z

native-platform/build.gradle

-            $.platforms.each { p ->
-                if (p.name.contains("ncurses")) {
-                    return
-                }
-                targetPlatform p.name
-            }


❓ The targetMachines are configured in the JniNokeePlugin which is a behaviour change. With the software model plugins, platforms need to be declared before using them. With Nokee, we simply create the target machine. We may want to consider a helper method that contains all of the default target machines and set the value here to make it more explicit.

lacasseio · 2021-11-27T19:11:25Z

native-platform/build.gradle

+        headers.from('src/shared/headers', 'build/generated/jni-headers/main')
+    }
+    variants.configureEach {
+        if (NcursesVersion.values().any { buildVariant.hasAxisOf(it) }) {


❓ 💄 We don't provide a way to check if an axis is present. Instead, queries are done by axis value. It could be useful to query the presence of an axis here however there are some additional open questions to consider.

Do you have a link to the latest docs for what the backing object for library and the element of variants?

It's a little bit out of date but it should give you a good enough idea: https://docs.nokee.dev/dsl/dev.nokee.platform.jni.JavaNativeInterfaceLibrary.html

lacasseio · 2021-11-27T19:15:23Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+        result.add(machines.os("osx").architecture("amd64"));
+        result.add(machines.os("osx").architecture("aarch64"));
+        result.add(machines.getLinux().architecture("amd64"));
+        result.add(machines.getLinux().architecture("aarch64"));
+        result.add(machines.getWindows().architecture("i386"));
+        result.add(machines.getWindows().architecture("amd64"));


🔍 We use osx and amd64 instead of the factory method getMacOs() and getX86_64() respectively simply to honour the previous naming. Both ad-hoc values are understood correctly. Using the canonical name would revert to stable names.

lacasseio · 2021-11-27T19:26:06Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+    private void configureCppTasks(Project project) {
+        TaskContainer tasks = project.getTasks();
+        TaskProvider<JavaCompile> compileJavaProvider = tasks.named("compileJava", JavaCompile.class);
+        tasks.withType(CppCompile.class)
+            .configureEach(task -> task.includes(
+                compileJavaProvider.flatMap(it -> it.getOptions().getHeaderOutputDirectory())
+            ));
+    }


🔍 We still wire the generated JNI headers manually because of two reasons: 1) the convention on Nokee source set needs to be reviewed and 2) the native sources are wired directly into the writeNativeVersionSources which the output are wired back to the compileJava causing a circular dependency.

Adding any additional files to the source set will override the convention. By default, the headers conversion for a JNI library with C++ sources will be src/<componentName>/headers and build/generated/jni-headers/<componentName>. When configuring the project, we mostly intend to add src/shared/headers instead of overwriting the current headers. However, given the circular dependency, the intention may be correct.

We could add an extra C source set just for the JNI headers but that would not be the correct intention. We need the JNI headers to compile any JNI native sources (C++).

I believe the ideal scenario would be to allow more flexibility in the source set so we can build upon the convention and filter the generated sources when configuring writeNativeVersionSources.

IIUC, we're doing this to ourselves.

We take the Java code and generate headers

We take the generated headers and create a hash. We put that into another generated header.

So to get the "set of generated headers" to generate the last header, we need something in between the full set of generated headers and the hand written stuff.

lacasseio · 2021-11-27T19:29:13Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+                // The core Gradle toolchain for Clang only targets x86 and x86_64 out of the box.
+                OperatingSystem os = new DefaultNativePlatform("current").getOperatingSystem();
+                if (os.isMacOsX()) {
+                    toolChain.target("macosaarch64");


❓ Target machines to target platform mapping use the canonical name. It seemed like a good idea at the time but in this context, it looks a bit strange. The target machine was declared using osx and we use macos here. The initial reason was to allow toolchain definition sharing regardless of the name chosen for the OS family and architecture. The reason is still very much valid.

lacasseio · 2021-11-27T19:38:09Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+import java.util.Set;
+
+@SuppressWarnings("UnstableApiUsage")
+public abstract class JniNokeePlugin implements Plugin<Project> {


❓ Is there a reason the JNI tests are run as part of another Test task? Nokee wires everything for the main Test task. There wasn't any consideration to "remove" the wiring of the JNI into the main test task. We can create a new test task but all the wiring will have to be done manually. Nokee could provide a jni-unit-test plugin that allows users to declare any number of JNI test tasks but that is a bit outside of the current scope. The project could remove the extra test task in favour of the main test task however there may be a reason for the task to existing in the first place.

We have some special tests where -Xcheck:jni output is treated as a reason for failure. These are the tests marked with JniChecksEnabled. For other tests that were introduced earlier, we did not enforce these checks yet, and they do fail.

One option to remove this distinction would be to make sure none of the current code emits any JNI warnings.

lacasseio · 2021-11-27T19:40:04Z

@big-guy The dependency verification is quite unhelpful. Locally, it fails with an Invalid UTF-8 error which is not clear as to what's the problem. I removed the verification files locally until we can work around this issue.

lacasseio · 2021-11-29T22:52:34Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+    private void registerWindowsDistributionDimension(JavaNativeInterfaceLibrary library) {
+        SetProperty<WindowsDistribution> newDimension = library.getDimensions().newAxis(WindowsDistribution.class, builder -> builder.onlyOn(library.getMachines().getWindows().getOperatingSystemFamily()));
+        newDimension.convention(ImmutableSet.copyOf(WindowsDistribution.values()));
+        ((ExtensionAware) library).getExtensions().add("targetWindowsDistributions", newDimension);


🔍 See conversation in the #ext-nokee Slack channel around ExtensionAware guideline.

Signed-off-by: Daniel Lacasse <[email protected]>

As of this commit, the axis is not present so the configuration has no effect. Signed-off-by: Daniel Lacasse <[email protected]>

Signed-off-by: Daniel Lacasse <[email protected]>

The Ncurses dimension is not configure as of this commit. However, the configuration will be the same. Signed-off-by: Daniel Lacasse <[email protected]>

We manually write the generated JNI headers to the C++ compile task instead of using the C++ source set as there seems to be a cycle in the configuration that uses writeNativeVersionSources. It's most likely due to up-to-date checks. Signed-off-by: Daniel Lacasse <[email protected]>

Signed-off-by: Daniel Lacasse <[email protected]>

They are wired in from within the JniNokeePlugin. Signed-off-by: Daniel Lacasse <[email protected]>

We also changed the named for the enum values so it better reflect the original naming. Signed-off-by: Daniel Lacasse <[email protected]>

Signed-off-by: Daniel Lacasse <[email protected]>

lacasseio · 2021-12-01T16:56:08Z

@big-guy I released an integration version. We still need to figure out the dependency verification error for CI to be happy.

big-guy · 2022-02-02T16:01:03Z

native-platform/build.gradle

+        headers.from('src/shared/headers', 'build/generated/jni-headers/main')
+    }
+    variants.configureEach {
+        if (NcursesVersion.values().any { buildVariant.hasAxisOf(it) }) {


Do you have a link to the latest docs for what the backing object for library and the element of variants?

big-guy · 2022-02-02T17:01:05Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+    private void configureCppTasks(Project project) {
+        TaskContainer tasks = project.getTasks();
+        TaskProvider<JavaCompile> compileJavaProvider = tasks.named("compileJava", JavaCompile.class);
+        tasks.withType(CppCompile.class)
+            .configureEach(task -> task.includes(
+                compileJavaProvider.flatMap(it -> it.getOptions().getHeaderOutputDirectory())
+            ));
+    }


IIUC, we're doing this to ourselves.

We take the Java code and generate headers

We take the generated headers and create a hash. We put that into another generated header.

So to get the "set of generated headers" to generate the last header, we need something in between the full set of generated headers and the hand written stuff.

big-guy · 2022-02-02T18:00:09Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+            } else {
+                return ImmutableList.of(toVariantName(it.getTargetMachine()));
+            }
+        }));


What does it mean to set the "variant names" here? This disables certain variants from running?

big-guy · 2022-02-02T18:01:34Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+            }
+        });
+        library.getVariants().configureEach(variant -> {
+            variant.getResourcePath().set(String.join("/",


What's this resource path relative to?

big-guy · 2022-02-02T18:03:14Z

buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java

+            toolChainRegistry.create("gcc", Gcc.class, toolChain -> {
+                // The core Gradle toolchain for gcc only targets x86 and x86_64 out of the box.
+                // https://github.com/gradle/gradle/blob/36614ee523e5906ddfa1fed9a5dc00a5addac1b0/subprojects/platform-native/src/main/java/org/gradle/nativeplatform/toolchain/internal/gcc/AbstractGccCompatibleToolChain.java
+                toolChain.target("linuxaarch64");


is there an issue for this?

lacasseio force-pushed the lacasseio/migration-to-nokee branch from 92f458f to 1e05f9c Compare November 27, 2021 18:56

lacasseio commented Nov 27, 2021

View reviewed changes

lacasseio force-pushed the lacasseio/migration-to-nokee branch from a436c79 to 73c1d92 Compare November 27, 2021 19:41

lacasseio commented Nov 29, 2021

View reviewed changes

lacasseio added 16 commits December 1, 2021 11:54

Add dependency to locally built Nokee plugins

6dc4273

Signed-off-by: Daniel Lacasse <[email protected]>

Add JNI Nokee plugin to replace the current JNI plugin

8f97efc

Signed-off-by: Daniel Lacasse <[email protected]>

First pass for building on current host (macOS)

fc6f1d6

Signed-off-by: Daniel Lacasse <[email protected]>

Configure target machines (OS and architecture)

080a58a

Signed-off-by: Daniel Lacasse <[email protected]>

Configure buildable variant names

bad86fa

Signed-off-by: Daniel Lacasse <[email protected]>

Add configuration of the main JAR POM

70ea6fa

Signed-off-by: Daniel Lacasse <[email protected]>

Respect onlyLocalVariants Gradle property

f328902

Signed-off-by: Daniel Lacasse <[email protected]>

Configure compiler and linker arguments

6285ad4

Signed-off-by: Daniel Lacasse <[email protected]>

Configure Windows distribution (WINDOWS_MIN)

4edc4b7

As of this commit, the axis is not present so the configuration has no effect. Signed-off-by: Daniel Lacasse <[email protected]>

Migrate file-events compiler and linker arguments

73dc4e8

Signed-off-by: Daniel Lacasse <[email protected]>

Fold ncurses and non-ncurses variant into single component

0a6cfa9

The Ncurses dimension is not configure as of this commit. However, the configuration will be the same. Signed-off-by: Daniel Lacasse <[email protected]>

Configure resource path to expected value (group + platform + variant)

50a8eee

Signed-off-by: Daniel Lacasse <[email protected]>

Remove reference to generated JNI headers

e4ebad4

They are wired in from within the JniNokeePlugin. Signed-off-by: Daniel Lacasse <[email protected]>

Register Windows distribution axis

a0f8491

We also changed the named for the enum values so it better reflect the original naming. Signed-off-by: Daniel Lacasse <[email protected]>

Use integration release instead of local snapshot

df95186

Signed-off-by: Daniel Lacasse <[email protected]>

lacasseio force-pushed the lacasseio/migration-to-nokee branch from b49d713 to df95186 Compare December 1, 2021 16:55

big-guy changed the base branch from master to nokee-migration December 23, 2021 14:13

big-guy merged commit 8054695 into gradle:nokee-migration Jan 5, 2022

big-guy reviewed Feb 2, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from core native to Nokee plugins #298

Migrate from core native to Nokee plugins #298

lacasseio commented Nov 27, 2021

lacasseio commented Nov 27, 2021

lacasseio Nov 27, 2021

lacasseio Nov 27, 2021

lacasseio Nov 27, 2021

lacasseio Nov 27, 2021

lacasseio Nov 27, 2021

big-guy Feb 2, 2022

lacasseio Feb 7, 2022

lacasseio Nov 27, 2021

lacasseio Nov 27, 2021

big-guy Feb 2, 2022

lacasseio Nov 27, 2021

lacasseio Nov 27, 2021

lptr Dec 16, 2021

lacasseio commented Nov 27, 2021

lacasseio Nov 29, 2021

lacasseio commented Dec 1, 2021

big-guy Feb 2, 2022

big-guy Feb 2, 2022

big-guy Feb 2, 2022

big-guy Feb 2, 2022

big-guy Feb 2, 2022


		import org.gradle.api.Named;

		public enum WindowsDistribution implements Named {

Migrate from core native to Nokee plugins #298

Migrate from core native to Nokee plugins #298

Conversation

lacasseio commented Nov 27, 2021

lacasseio commented Nov 27, 2021

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

lacasseio commented Nov 27, 2021

Choose a reason for hiding this comment

lacasseio commented Dec 1, 2021

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