Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from core native to Nokee plugins #298

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions buildSrc/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ plugins {
repositories {
jcenter()
gradlePluginPortal()
maven { url = 'https://repo.nokee.dev/snapshot' }
}

dependencies {
implementation gradleApi()
implementation platform('dev.nokee:nokee-gradle-plugins:0.4.1229-202112011142.4a01703c')
implementation 'org.apache.httpcomponents.client5:httpclient5:5.0.1'
implementation 'com.google.guava:guava:28.2-jre'
implementation 'org.gradle:test-retry-gradle-plugin:1.1.8'
Expand All @@ -30,6 +32,10 @@ gradlePlugin {
id = "gradlebuild.jni"
implementationClass = "gradlebuild.JniPlugin"
}
jniNokee {
id = "gradlebuild.jni-nokee"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

implementationClass = "gradlebuild.JniNokeePlugin"
}
nativeComponent {
id = "gradlebuild.native-platform-component"
implementationClass = "gradlebuild.NativePlatformComponentPlugin"
Expand Down
213 changes: 213 additions & 0 deletions buildSrc/src/main/java/gradlebuild/JniNokeePlugin.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
package gradlebuild;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import dev.nokee.language.cpp.CppSourceSet;
import dev.nokee.platform.jni.JavaNativeInterfaceLibrary;
import dev.nokee.runtime.nativebase.TargetMachine;
import dev.nokee.runtime.nativebase.TargetMachineFactory;
import groovy.util.Node;
import org.gradle.api.Plugin;
import org.gradle.api.Project;
import org.gradle.api.plugins.ExtensionAware;
import org.gradle.api.plugins.JavaPluginConvention;
import org.gradle.api.provider.SetProperty;
import org.gradle.api.publish.PublishingExtension;
import org.gradle.api.publish.maven.MavenPublication;
import org.gradle.api.tasks.SourceSet;
import org.gradle.api.tasks.TaskContainer;
import org.gradle.api.tasks.TaskProvider;
import org.gradle.api.tasks.compile.JavaCompile;
import org.gradle.language.cpp.tasks.CppCompile;
import org.gradle.model.Mutate;
import org.gradle.model.RuleSource;
import org.gradle.nativeplatform.platform.OperatingSystem;
import org.gradle.nativeplatform.platform.internal.DefaultNativePlatform;
import org.gradle.nativeplatform.tasks.LinkSharedLibrary;
import org.gradle.nativeplatform.toolchain.Clang;
import org.gradle.nativeplatform.toolchain.Gcc;
import org.gradle.nativeplatform.toolchain.NativeToolChainRegistry;
import org.gradle.nativeplatform.toolchain.VisualCpp;

import java.io.File;
import java.util.Set;

@SuppressWarnings("UnstableApiUsage")
public abstract class JniNokeePlugin implements Plugin<Project> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Override
public void apply(Project project) {
project.getPluginManager().apply(NativePlatformComponentPlugin.class);
VariantsExtension variants = project.getExtensions().getByType(VariantsExtension.class);
project.getPluginManager().apply("dev.nokee.jni-library");
project.getPluginManager().apply(NativeToolChainRules.class);

configureCppTasks(project);
configureMainLibrary(project);
configureVariants(project);
addComponentSourcesSetsToProjectSourceSet(project.getTasks(), project.getExtensions().getByType(JavaNativeInterfaceLibrary.class));

configureNativeVersionGeneration(project);

configurePomOfMainJar(project, variants);
}

private void configureVariants(Project project) {
JavaNativeInterfaceLibrary library = project.getExtensions().getByType(JavaNativeInterfaceLibrary.class);
VariantsExtension variants = project.getExtensions().getByType(VariantsExtension.class);
variants.getVariantNames().set(library.getVariants().flatMap(it -> {
// Only depend on variants which can be built on the current machine
boolean onlyLocalVariants = project.getProviders().gradleProperty("onlyLocalVariants").forUseAtConfigurationTime().isPresent();
if (onlyLocalVariants && !it.getSharedLibrary().isBuildable()) {
return ImmutableList.of();
} else {
return ImmutableList.of(toVariantName(it.getTargetMachine()));
}
}));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private static String toVariantName(TargetMachine targetMachine) {
return targetMachine.getOperatingSystemFamily().getName() + "-" + targetMachine.getArchitecture().getName();
}

private void configureMainLibrary(Project project) {
JavaNativeInterfaceLibrary library = project.getExtensions().getByType(JavaNativeInterfaceLibrary.class);
registerWindowsDistributionDimension(library);
library.getTargetMachines().set(supportedMachines(library.getMachines()));
library.getTasks().configureEach(CppCompile.class, task -> {
task.getCompilerArgs().addAll(task.getTargetPlatform().map(targetPlatform -> {
OperatingSystem targetOs = targetPlatform.getOperatingSystem();
if (targetOs.isMacOsX()) {
return ImmutableList.of("-mmacosx-version-min=10.9");
} else if (targetOs.isLinux()) {
return ImmutableList.of("-D_FILE_OFFSET_BITS=64");
} else {
return ImmutableList.of(); // do nothing
}
}));
});
library.getTasks().configureEach(LinkSharedLibrary.class, task -> {
task.getLinkerArgs().addAll(task.getTargetPlatform().map(targetPlatform -> {
OperatingSystem targetOs = targetPlatform.getOperatingSystem();
if (targetOs.isMacOsX()) {
return ImmutableList.of(
"-mmacosx-version-min=10.9",
"-framework", "CoreServices");
} else if (targetOs.isWindows()) {
return ImmutableList.of("Shlwapi.lib", "Advapi32.lib");
} else {
return ImmutableList.of(); // do nothing
}
}));
});
library.getVariants().configureEach(variant -> {
if (variant.getBuildVariant().hasAxisOf(WindowsDistribution.WINDOWS_XP_OR_LOWER)) {
variant.getTasks().configureEach(CppCompile.class, task -> {
task.getCompilerArgs().add("/DWINDOWS_MIN");
});
}
});
library.getVariants().configureEach(variant -> {
variant.getResourcePath().set(String.join("/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this resource path relative to?

project.getGroup().toString().replace('.', '/'),
"platform",
toVariantName(variant.getTargetMachine())));
});
}

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);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private void addComponentSourcesSetsToProjectSourceSet(TaskContainer tasks, JavaNativeInterfaceLibrary library) {
tasks.withType(WriteNativeVersionSources.class, task -> {
task.getNativeSources().from(library.getSources().flatMap(sourceSet -> {
if (sourceSet instanceof CppSourceSet) {
return ImmutableList.of(sourceSet.getSourceDirectories(), ((CppSourceSet) sourceSet).getHeaders().getSourceDirectories());
} else {
return ImmutableList.of();
}
}));
});
}

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())
));
}
Comment on lines +136 to +143
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private static Set<TargetMachine> supportedMachines(TargetMachineFactory machines) {
ImmutableSet.Builder<TargetMachine> result = ImmutableSet.builder();
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"));
Comment on lines +147 to +152
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

return result.build();
}

private void configureNativeVersionGeneration(Project project) {
NativePlatformVersionExtension nativeVersion = project.getExtensions().create("nativeVersion", NativePlatformVersionExtension.class);

File generatedFilesDir = new File(project.getBuildDir(), "generated");

TaskProvider<WriteNativeVersionSources> writeNativeVersionSources = project.getTasks().register("writeNativeVersionSources", WriteNativeVersionSources.class, task -> {
task.getGeneratedNativeHeaderDirectory().set(new File(generatedFilesDir, "version/header"));
task.getGeneratedJavaSourcesDir().set(new File(generatedFilesDir, "version/java"));
task.getVersionClassPackageName().set(nativeVersion.getVersionClassPackageName());
task.getVersionClassName().set(nativeVersion.getVersionClassName());
});


project.getTasks().withType(CppCompile.class).configureEach(task ->
task.includes(writeNativeVersionSources.flatMap(WriteNativeVersionSources::getGeneratedNativeHeaderDirectory)
));
JavaPluginConvention javaPluginConvention = project.getConvention().getPlugin(JavaPluginConvention.class);
javaPluginConvention.getSourceSets().getByName(SourceSet.MAIN_SOURCE_SET_NAME).java(javaSources ->
javaSources.srcDir(writeNativeVersionSources.flatMap(WriteNativeVersionSources::getGeneratedJavaSourcesDir))
);
}

private void configurePomOfMainJar(Project project, VariantsExtension variants) {
project.getExtensions().configure(
PublishingExtension.class,
extension -> extension.getPublications().named("main", MavenPublication.class, main ->
main.getPom().withXml(xmlProvider -> {
Node node = xmlProvider.asNode();
Node deps = node.appendNode("dependencies");
variants.getVariantNames().get().forEach(variantName -> {
Node dep = deps.appendNode("dependency");
dep.appendNode("groupId", project.getGroup());
dep.appendNode("artifactId", main.getArtifactId() + "-" + variantName);
dep.appendNode("version", project.getVersion());
dep.appendNode("scope", "runtime");
});
})));
}

public static class NativeToolChainRules extends RuleSource {
@Mutate
void createToolChains(NativeToolChainRegistry toolChainRegistry) {
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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an issue for this?

});
toolChainRegistry.create("clang", Clang.class, toolChain -> {
// 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");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ 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.

}
});
toolChainRegistry.create("visualCpp", VisualCpp.class);
}
}
}
18 changes: 18 additions & 0 deletions buildSrc/src/main/java/gradlebuild/NcursesVersion.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package gradlebuild;

import org.gradle.api.Named;

public enum NcursesVersion implements Named {
NCURSES_5("5"), NCURSES_6("6");

private final String versionNumber;

NcursesVersion(String versionNumber) {
this.versionNumber = versionNumber;
}

@Override
public String getName() {
return "ncurses" + versionNumber;
}
}
18 changes: 18 additions & 0 deletions buildSrc/src/main/java/gradlebuild/WindowsDistribution.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package gradlebuild;

import org.gradle.api.Named;

public enum WindowsDistribution implements Named {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

WINDOWS_XP_OR_LOWER {
@Override
public String getName() {
return "min";
}
},
WINDOWS_VISTA_OR_HIGHER {
@Override
public String getName() {
return "";
}
};
}
99 changes: 55 additions & 44 deletions file-events/build.gradle
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
plugins {
id 'groovy'
id 'cpp'
id 'gradlebuild.jni'
id 'dev.nokee.cpp-language'
id 'gradlebuild.jni-nokee'
}

import dev.nokee.language.cpp.CppSourceSet

nativeVersion {
versionClassPackageName = "net.rubygrapefruit.platform.internal.jni"
versionClassName = "FileEventsVersion"
Expand All @@ -19,49 +21,58 @@ javadoc {
exclude '**/internal/**'
}

model {
components {
nativePlatformFileEvents(NativeLibrarySpec) {
baseName 'native-platform-file-events'
$.platforms.each { p ->
targetPlatform p.name
}
binaries.all {
if (targetPlatform.operatingSystem.macOsX
|| targetPlatform.operatingSystem.linux) {
cppCompiler.args "-g" // Produce debug output
cppCompiler.args "-pthread" // Force nicer threading
cppCompiler.args "-pedantic" // Disable non-standard things
cppCompiler.args "--std=c++11" // Enable C++11
cppCompiler.args "-Wall" // All warnings
cppCompiler.args "-Wextra" // Plus extra
cppCompiler.args "-Wformat=2" // Check printf format strings
cppCompiler.args "-Werror" // Warnings are errors
cppCompiler.args "-Wno-format-nonliteral" // Allow printf to have dynamic format string
cppCompiler.args "-Wno-unguarded-availability-new" // Newly introduced flags are not available on older macOS versions
linker.args "-pthread"
} else if (targetPlatform.operatingSystem.windows) {
cppCompiler.args "/DEBUG" // Produce debug output
cppCompiler.args "/std:c++17" // Won't hurt
cppCompiler.args "/permissive-" // Make compiler more standards compatible
cppCompiler.args "/EHsc" // Force exception handling mode
cppCompiler.args "/Zi" // Force PDB debugging
cppCompiler.args "/FS" // Force synchronous PDB writes
cppCompiler.args "/Zc:inline" // Hack
cppCompiler.args "/Zc:throwingNew" // Assume new throws on error
cppCompiler.args "/W3" // Enable lots of warnings, disbale individual warnings with /WD<NUM>
cppCompiler.args "/WX" // Warnings are errors
cppCompiler.args "/D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING"
// Don't issue warnings for wstring_convert in generic_fsnotifier.cpp
linker.args "/DEBUG:FULL" // Generate all PDBs
}
library {
baseName = 'native-platform-file-events'
sources.configureEach(CppSourceSet) {
source.from('src/file-events/cpp')
headers.from('src/file-events/headers')
}
tasks.configureEach(CppCompile) {
compilerArgs.addAll(targetPlatform.map {
if (it.operatingSystem.macOsX
|| it.operatingSystem.linux) {
[
"-g", // Produce debug output
"-pthread", // Force nicer threading
"-pedantic", // Disable non-standard things
"--std=c++11", // Enable C++11
"-Wall", // All warnings
"-Wextra", // Plus extra
"-Wformat=2", // Check printf format strings
"-Werror", // Warnings are errors
"-Wno-format-nonliteral", // Allow printf to have dynamic format string
"-Wno-unguarded-availability-new", // Newly introduced flags are not available on older macOS versions
]
} else if (it.operatingSystem.windows) {
[
"/DEBUG", // Produce debug output
"/std:c++17", // Won't hurt
"/permissive-", // Make compiler more standards compatible
"/EHsc", // Force exception handling mode
"/Zi", // Force PDB debugging
"/FS", // Force synchronous PDB writes
"/Zc:inline", // Hack
"/Zc:throwingNew", // Assume new throws on error
"/W3", // Enable lots of warnings, disbale individual warnings with /WD<NUM>
"/WX", // Warnings are errors
"/D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING",
// Don't issue warnings for wstring_convert in generic_fsnotifier.cpp
]
} else {
[]
}
sources {
cpp {
source.srcDirs = ['src/file-events/cpp']
exportedHeaders.srcDirs = ['src/file-events/headers']
}
})
}
tasks.configureEach(LinkSharedLibrary) {
linkerArgs.addAll(targetPlatform.map {
if (it.operatingSystem.macOsX
|| it.operatingSystem.linux) {
["-pthread"]
} else if (it.operatingSystem.windows) {
["/DEBUG:FULL"] // Generate all PDBs
} else {
[]
}
}
})
}
}
Loading