[jnigen] General cleanup (#80)

Author: mahesh-hegde

.github/workflows/test-package.yml

@@ -121,11 +121,18 @@ jobs:
         with:
           distribution: 'zulu'
           java-version: '11'
+      - name: install clang tools
+        run: |
+          sudo apt-get update -y
+          sudo apt-get install -y clang-format
       - run: flutter pub get
       - name: Check formatting
         run: flutter format --output=none --set-exit-if-changed .
       - name: Run lints
         run: flutter analyze --fatal-infos
+      - name: Check C code formatting using clang-format
+        run: clang-format --dry-run -Werror dartjni.c dartjni.h third_party/*.c third_party/*.h
+        working-directory: pkg/jni/src
 
   test_jni:
     runs-on: ubuntu-latest
@@ -192,7 +199,9 @@ jobs:
       - run: Add-Content $env:GITHUB_PATH "$env:JAVA_HOME\bin\server"
       - run: dart pub get
       - run: dart run jni:setup
-      - run: dart test
+      ## TODO(#51): -j 1 is required to avoid a rare race condition which is caused by
+      ## all tests which spawn JVM being run in same process.
+      - run: dart test -j 1
 
   test_jnigen_windows_minimal:
     needs: [analyze_jnigen]
@@ -362,7 +371,7 @@ jobs:
           diff -qr _c src/
           diff -qr _dart lib/third_party
       - name: Generate full bindings
-        run: dart run jnigen --config jnigen_full.yaml
+        run: dart run jnigen --config jnigen.yaml --override classes="org.apache.pdfbox.pdmodel;org.apache.pdfbox.text"
       - name: Analyze generated bindings
         run: |
           flutter pub get # dart-analyze errors on flutter example

pkgs/jni/README.md

@@ -17,7 +17,7 @@ This is intended for one-off / debugging uses of JNI, as well as providing a bas
 __To generate type-safe bindings from Java libraries, use `jnigen`.__
 
 ## SDK Note
-Dart standalone is supported, but due to some current limitations of the `pubspec` format, `dart` command must be from Flutter SDK and not dart SDK. See dart-lang/pub#3563.
+Dart standalone is supported, but due to some current limitations of the `pubspec` format, `dart` command must be from Flutter SDK and not dart SDK.
 
 ## Version note
 This library is at an early stage of development and we do not provide backwards compatibility of the API at this point.

pkgs/jni/android/README.md

@@ -0,0 +1 @@
+This folder contains the [Android plugin class](src/main/java/com/github/dart_lang/jni/JniPlugin.java) which takes care of initializing JNI on Android.

pkgs/jni/bin/setup.dart

@@ -7,111 +7,141 @@ import 'dart:io';
 import 'package:args/args.dart';
 import 'package:package_config/package_config.dart';
 
-const _buildDir = "build-dir";
-const _srcDir = "source-dir";
+const ansiRed = '\x1b[31m';
+const ansiDefault = '\x1b[39;49m';
+
+const _defaultRelativeBuildPath = "build/jni_libs";
+
+const _buildPath = "build-path";
+const _srcPath = "source-path";
 const _packageName = 'package-name';
 const _verbose = "verbose";
 const _cmakeArgs = "cmake-args";
-const _clean = "clean";
-
-const _cmakeTemporaryFiles = [
-  'CMakeCache.txt',
-  'CMakeFiles/',
-  'cmake_install.cmake',
-  'Makefile'
-];
-
-void deleteCMakeTemps(Uri buildDir) async {
-  for (var filename in _cmakeTemporaryFiles) {
-    if (options.verbose) {
-      stderr.writeln('remove $filename');
-    }
-    await File(buildDir.resolve(filename).toFilePath()).delete(recursive: true);
+
+Future<void> runCommand(
+    String exec, List<String> args, String workingDir) async {
+  // For printing relative path always.
+  var current = Directory.current.path;
+  if (!current.endsWith(Platform.pathSeparator)) {
+    current += Platform.pathSeparator;
+  }
+  if (workingDir.startsWith(current)) {
+    workingDir.replaceFirst(current, "");
   }
-}
 
-// Sets up input output channels and maintains state.
-class CommandRunner {
-  CommandRunner({this.printCmds = false});
-  bool printCmds = false;
-  int? time;
-  Future<CommandRunner> run(
-      String exec, List<String> args, String workingDir) async {
-    if (printCmds) {
-      final cmd = "$exec ${args.join(" ")}";
-      stderr.writeln("\n+ [$workingDir] $cmd");
-    }
-    final process = await Process.start(exec, args,
-        workingDirectory: workingDir,
-        runInShell: true,
-        mode: ProcessStartMode.inheritStdio);
-    final exitCode = await process.exitCode;
+  final cmd = "$exec ${args.join(" ")}";
+  stderr.writeln("+ [$workingDir] $cmd");
+  int exitCode;
+  if (options.verbose) {
+    final process = await Process.start(
+      exec, args,
+      workingDirectory: workingDir,
+      mode: ProcessStartMode.inheritStdio,
+      // without `runInShell`, sometimes cmake doesn't run on windows.
+      runInShell: true,
+    );
+    exitCode = await process.exitCode;
+  } else {
+    // ProcessStartMode.normal sometimes hangs on windows. No idea why.
+    final process = await Process.run(exec, args,
+        runInShell: true, workingDirectory: workingDir);
+    exitCode = process.exitCode;
     if (exitCode != 0) {
-      stderr.writeln("command exited with $exitCode");
+      var out = process.stdout;
+      var err = process.stderr;
+      if (stdout.supportsAnsiEscapes) {
+        out = "$ansiRed$out$ansiDefault";
+        err = "$ansiRed$err$ansiDefault";
+      }
+      stdout.writeln(out);
+      stderr.writeln(err);
     }
-    return this;
+  }
+  if (exitCode != 0) {
+    stderr.writeln("Command exited with $exitCode.");
   }
 }
 
 class Options {
   Options(ArgResults arg)
-      : buildDir = arg[_buildDir],
-        srcDir = arg[_srcDir],
+      : buildPath = arg[_buildPath],
+        srcPath = arg[_srcPath],
         packageName = arg[_packageName] ?? 'jni',
         cmakeArgs = arg[_cmakeArgs],
-        verbose = arg[_verbose] ?? false,
-        clean = arg[_clean] ?? false;
+        verbose = arg[_verbose] ?? false;
 
-  String? buildDir, srcDir;
+  String? buildPath, srcPath;
   String packageName;
   List<String> cmakeArgs;
-  bool verbose, clean;
+  bool verbose;
 }
 
 late Options options;
-void log(String msg) {
+
+void verboseLog(String msg) {
   if (options.verbose) {
     stderr.writeln(msg);
   }
 }
 
-/// tries to find package:jni's source folder in pub cache
-/// if not possible, returns null.
-Future<String?> findSources() async {
+/// Find path to C sources in pub cache for package specified by [packageName].
+///
+/// It's assumed C FFI sources are in "src/" relative to package root.
+/// If package cannot be found, null is returned.
+Future<String?> findSources(String packageName) async {
   final packageConfig = await findPackageConfig(Directory.current);
   if (packageConfig == null) {
     return null;
   }
-  final packages = packageConfig.packages;
-  for (var package in packages) {
-    if (package.name == options.packageName) {
-      return package.root.resolve("src/").toFilePath();
+  final package = packageConfig[options.packageName];
+  if (package == null) {
+    return null;
+  }
+  return package.root.resolve("src").toFilePath();
+}
+
+/// Returns true if [artifact] does not exist, or any file in [sourceDir] is
+/// newer than [artifact].
+bool needsBuild(File artifact, Directory sourceDir) {
+  if (!artifact.existsSync()) return true;
+  final fileLastModified = artifact.lastModifiedSync();
+  for (final entry in sourceDir.listSync(recursive: true)) {
+    if (entry.statSync().modified.isAfter(fileLastModified)) {
+      return true;
     }
   }
-  return null;
+  return false;
+}
+
+/// Returns the name of file built using sources in [cDir]
+String getTargetName(Directory cDir) {
+  for (final file in cDir.listSync(recursive: true)) {
+    if (file.path.endsWith(".c")) {
+      final cFileName = file.uri.pathSegments.last;
+      final librarySuffix = Platform.isWindows ? "dll" : "so";
+      return cFileName.substring(0, cFileName.length - 1) + librarySuffix;
+    }
+  }
+  throw Exception("Could not find a C file in ${cDir.path}");
 }
 
 void main(List<String> arguments) async {
   final parser = ArgParser()
-    ..addOption(_buildDir,
-        abbr: 'B', help: 'Directory to place built artifacts')
-    ..addOption(_srcDir,
-        abbr: 'S', help: 'alternative path to package:jni sources')
+    ..addOption(_buildPath,
+        abbr: 'b', help: 'Directory to place built artifacts')
+    ..addOption(_srcPath,
+        abbr: 's', help: 'alternative path to package:jni sources')
     ..addOption(_packageName,
         abbr: 'p',
         help: 'package for which native'
             'library should be built',
         defaultsTo: 'jni')
     ..addFlag(_verbose, abbr: 'v', help: 'Enable verbose output')
-    ..addFlag(_clean,
-        negatable: false,
-        abbr: 'C',
-        help: 'Clear built artifacts instead of running a build')
     ..addMultiOption(_cmakeArgs,
-        abbr: 'm', help: 'additional argument to pass to CMake');
-  final cli = parser.parse(arguments);
-  options = Options(cli);
-  final rest = cli.rest;
+        abbr: 'm', help: 'Pass additional argument to CMake');
+  final argResults = parser.parse(arguments);
+  options = Options(argResults);
+  final rest = argResults.rest;
 
   if (rest.isNotEmpty) {
     stderr.writeln("one or more unrecognized arguments: $rest");
@@ -121,79 +151,69 @@ void main(List<String> arguments) async {
     return;
   }
 
-  final srcPath = options.srcDir ?? await findSources();
+  final srcPath = options.srcPath ?? await findSources(options.packageName);
 
   if (srcPath == null) {
-    stderr.writeln("No sources specified and current directory is not a "
-        "package root.");
+    stderr.writeln("Cannot find sources for package ${options.packageName} "
+        "and no sources were manually specified.");
     exitCode = 1;
     return;
   }
 
   final srcDir = Directory(srcPath);
-  if (!await srcDir.exists() && !options.clean) {
-    throw 'Directory $srcPath does not exist';
+  if (!srcDir.existsSync()) {
+    stderr.writeln('Directory $srcPath does not exist');
+    exitCode = 1;
+    return;
   }
 
-  log("srcPath: $srcPath");
+  verboseLog("srcPath: $srcPath");
 
-  final currentDirUri = Uri.file(".");
-  final buildPath =
-      options.buildDir ?? currentDirUri.resolve("build/jni_libs").toFilePath();
+  final currentDirUri = Uri.directory(".");
+  final buildPath = options.buildPath ??
+      currentDirUri.resolve(_defaultRelativeBuildPath).toFilePath();
   final buildDir = Directory(buildPath);
   await buildDir.create(recursive: true);
-  log("buildPath: $buildPath");
+  verboseLog("buildPath: $buildPath");
 
   if (buildDir.absolute.uri == srcDir.absolute.uri) {
     stderr.writeln("Please build in a directory different than source.");
-    exit(2);
+    exitCode = 1;
+    return;
   }
 
-  if (options.clean) {
-    await cleanup(options, srcDir.absolute.path, buildDir.absolute.path);
-  } else {
-    // pass srcDir absolute path because it will be passed to CMake as arg
-    // which will be running in different directory
-    final jniDirUri = Uri.directory(".dart_tool").resolve("jni");
-    final jniDir = Directory.fromUri(jniDirUri);
-    await jniDir.create(recursive: true);
-    final tempDir = await jniDir.createTemp("jni_native_build_");
-    await build(options, srcDir.absolute.path, tempDir.path);
-    final dllDirUri =
-        Platform.isWindows ? tempDir.uri.resolve("Debug") : tempDir.uri;
-    final dllDir = Directory.fromUri(dllDirUri);
-    for (var entry in dllDir.listSync()) {
-      final dllSuffix = Platform.isWindows ? "dll" : "so";
-      if (entry.path.endsWith(dllSuffix)) {
-        final dllName = entry.uri.pathSegments.last;
-        final target = buildDir.uri.resolve(dllName);
-        entry.renameSync(target.toFilePath());
-      }
-    }
-    await tempDir.delete(recursive: true);
+  final targetFileUri = buildDir.uri.resolve(getTargetName(srcDir));
+  final targetFile = File.fromUri(targetFileUri);
+  if (!needsBuild(targetFile, srcDir)) {
+    verboseLog("last modified of ${targetFile.path}: "
+        "${targetFile.lastModifiedSync()}");
+    stderr.writeln("target newer than source, skipping build");
+    return;
   }
-}
 
-Future<void> build(Options options, String srcPath, String buildPath) async {
-  final runner = CommandRunner(printCmds: true);
+  // Note: creating temp dir in .dart_tool/jni instead of SystemTemp
+  // because latter can fail tests on Windows CI, when system temp is on
+  // separate drive or something.
+  final jniDirUri = Uri.directory(".dart_tool").resolve("jni");
+  final jniDir = Directory.fromUri(jniDirUri);
+  await jniDir.create(recursive: true);
+  final tempDir = await jniDir.createTemp("jni_native_build_");
   final cmakeArgs = <String>[];
   cmakeArgs.addAll(options.cmakeArgs);
-  cmakeArgs.add(srcPath);
-  await runner.run("cmake", cmakeArgs, buildPath);
-  await runner.run("cmake", ["--build", "."], buildPath);
-}
-
-Future<void> cleanup(Options options, String srcPath, String buildPath) async {
-  if (srcPath == buildPath) {
-    stderr.writeln('Error: build path is same as source path.');
-  }
-
-  stderr.writeln("deleting $buildPath");
-
-  try {
-    await Directory(buildPath).delete(recursive: true);
-  } catch (e) {
-    stderr.writeln("Error: cannot be deleted");
-    stderr.writeln(e);
+  // Pass absolute path of srcDir because cmake command is run in temp dir
+  cmakeArgs.add(srcDir.absolute.path);
+  await runCommand("cmake", cmakeArgs, tempDir.path);
+  await runCommand("cmake", ["--build", "."], tempDir.path);
+  final dllDirUri =
+      Platform.isWindows ? tempDir.uri.resolve("Debug") : tempDir.uri;
+  final dllDir = Directory.fromUri(dllDirUri);
+  for (var entry in dllDir.listSync()) {
+    final dllSuffix = Platform.isWindows ? "dll" : "so";
+    if (entry.path.endsWith(dllSuffix)) {
+      final dllName = entry.uri.pathSegments.last;
+      final target = buildDir.uri.resolve(dllName);
+      entry.renameSync(target.toFilePath());
+    }
   }
+  await tempDir.delete(recursive: true);
 }