Check that the loaded native library is compatible [ECR-3148]: #882

dmitry-timofeev · 2019-05-08T18:13:43Z

Overview

Check that the loaded native library is compatible, i.e.,
has the same version as the Java library. This check is useful in
service integration tests that can update their dependency
on 'exonum-java-binding-core' but not the application (that
contains the native library), or vice versa.

Requires native code to enable this check: ECR-3172

See: https://jira.bf.local/browse/ECR-3148

Definition of Done

There are no TODOs left in the code
Change is covered by automated tests
The coding guidelines are followed
Public API has Javadoc
Method preconditions are checked and documented in the Javadoc of the method
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

Check that the loaded native library is compatible, i.e., has the same version as the Java library. This check is useful in _service integration tests_ that can update their dependency on 'exonum-java-binding-core' but not the application (that contains the native library), or vice versa. Requires native code to enable this check: ECR-3172

dmitry-timofeev · 2019-05-08T18:21:04Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java

+    }
+  }
+
+  // todo: Shall we extract it as a Supplier<String>, and System.load() as Consumer<String>


We can possibly parameterize this class with this couple of functions (hence the constructor):

private static final LibraryLoader INSTANCE = new LibraryLoader(System::loadLibrary, LibraryLoader::nativeGetLibraryVersion, JAVA_BINDING_VERSION);

Is this complication worth it? Also, it will still have dependencies on the system properties and environment, unless we also abstract them and parameterize 🙃

I'm not sure about such refactoring for tests because it contains only one equals statement.

🙆‍♂️ , let's keep it simple(r) then.

dmitry-timofeev · 2019-05-08T18:22:33Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java

  private static final String JAVA_LIBRARY_PATH_PROPERTY = "java.library.path";
  private static final String DYNAMIC_LIBRARIES_ENV_VAR_WINDOWS = "PATH";
  private static final String DYNAMIC_LIBRARIES_ENV_VAR_UNIX = "LD_LIBRARY_PATH";
+  // TODO: Shall this class read the properties instead?


We could make Maven auto-generate the properties file with the version, but … not sure we need that at this moment (with some code here reading those properties).

should we then update the version string here on each release?

Yes — update this string each release or make Maven write and this code read the build properties.

Would it be possible/better to generate source file with constants before building the "static" source code? (move it to compilation phase instead of reading properties at runtime)

It's trivial to pick up maven properties in runtime. So I believe it brings more benefits than drawbacks 🦊
Also, looks like we can configure RUSTFLAGS in maven and remove tests_profile? 🤔

It's trivial to pick up maven properties in runtime. So I believe it brings more benefits than drawbacks fox_face

Yes, it is not difficult at all, but it seems like an overkill at this point: this class will gain a responsibility to read (once) a certain file with properties + the build configuration needs to be updated.

Also, looks like we can configure RUSTFLAGS in maven and remove tests_profile

How? It invokes rustup and find do determine some various paths to add to RUSTFLAGS.

For example:

<plugins> <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-antrun-plugin</artifactId> <version>1.7</version> <executions> <execution> <id>define-property</id> <phase>validate</phase> <goals> <goal>run</goal> </goals> <configuration> <exportAntProperties>true</exportAntProperties> <tasks> <exec executable="git" outputproperty="testproperty"> <arg value="--version"/> </exec> </tasks> </configuration> </execution> <execution> <id>show-property</id> <phase>compile</phase> <goals> <goal>run</goal> </goals> <configuration> <tasks> <echo>Displaying value of 'testproperty':</echo> <echo>[testproperty] ${testproperty}</echo> </tasks> </configuration> </execution> </executions> </plugin> </plugins>

It'll print in the log:

[echo] Displaying value of 'testproperty': [echo] [testproperty] git version 2.20.1 (Apple Git-117)

Looks interesting, maybe we can offload that to an Ant script instead of a bash script for ease of use (no source tests_profile) and support of any platform (Windows). Please file an issue if consider that promising.

ECR-3184 ECR-3186

coveralls · 2019-05-08T18:38:04Z

Coverage increased (+0.04%) to 86.077% when pulling 94af3d7 on dmitry-timofeev:check-native-library-version-ECR-3148 into 201f5fc on exonum:master.

[skip ci]

skletsun · 2019-05-13T10:07:36Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java

- * A loader of the native shared library with Exonum framework bindings.
+ * A loader of the native shared library with Exonum framework bindings. It loads the native
+ * library and also verifies that it is compatible with the Java classes. The native library
+ * is compatible iff it has exactly the same version as this Java library. The revision


Typo: iff -> if

It isn't: "if and only if"

https://en.oxforddictionaries.com/definition/iff

bullet-tooth · 2019-05-13T11:57:13Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java

+   * @param libraryVersion the version of this library to verify that the native library
+   *     is compatible with it. The compatibility is d
+   */
+  LibraryLoader(String libraryVersion) {


why does it package private, not private? I see no tests.

bullet-tooth · 2019-05-13T11:57:40Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java

+   * Creates a new library loader.
+   *
+   * @param libraryVersion the version of this library to verify that the native library
+   *     is compatible with it. The compatibility is d


probably "determined by comparing …" :-D

code review gives you great opportunity to discover lots of new words like "iff", "d" 🙃

@skletsun OMG YOLO

bullet-tooth · 2019-05-13T11:58:49Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java

  private static final String JAVA_LIBRARY_PATH_PROPERTY = "java.library.path";
  private static final String DYNAMIC_LIBRARIES_ENV_VAR_WINDOWS = "PATH";
  private static final String DYNAMIC_LIBRARIES_ENV_VAR_UNIX = "LD_LIBRARY_PATH";
+  // TODO: Shall this class read the properties instead?


should we then update the version string here on each release?

bullet-tooth · 2019-05-13T12:01:16Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java


  private static final Logger logger = LogManager.getLogger(LibraryLoader.class);

+  private static final LibraryLoader INSTANCE = new LibraryLoader(JAVA_BINDING_VERSION);


How about to implement the singleton with lazy loading? It doesn't bring a big overhead but anyway.

But it will never be actually "lazy" loaded, if I understand correctly — a class is loaded when its first static symbol is accessed (with some exception for compile-time constants, I believe), and the only publicly accessible symbol is load.

https://docs.oracle.com/javase/specs/jls/se11/html/jls-12.html#jls-12.4.1

Agree, you are right. My doubt was that it'll be loaded in case no integration tests defined.

bullet-tooth · 2019-05-13T12:01:52Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java

  }

-  private static void loadOnce() {
+  synchronized void loadOnce() {


bullet-tooth · 2019-05-13T12:04:38Z

exonum-java-binding/core/src/main/java/com/exonum/binding/util/LibraryLoader.java

+    }
+  }
+
+  // todo: Shall we extract it as a Supplier<String>, and System.load() as Consumer<String>


I'm not sure about such refactoring for tests because it contains only one equals statement.

…version-ECR-3148

dmitry-timofeev requested review from skletsun, dip56, bullet-tooth, MakarovS and vitvakatu May 8, 2019 18:13

dmitry-timofeev commented May 8, 2019

View reviewed changes

dmitry-timofeev added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label May 8, 2019

Merge branch 'master' into check-native-library-version-ECR-3148

1004585

[skip ci]

skletsun reviewed May 13, 2019

View reviewed changes

bullet-tooth reviewed May 13, 2019

View reviewed changes

dmitry-timofeev added 3 commits May 14, 2019 15:42

Fix review comments

ccb29f8

Add changelog

7b25886

Merge remote-tracking branch 'main/master' into check-native-library-…

77b4ef8

…version-ECR-3148

dmitry-timofeev removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label May 14, 2019

Merge remote-tracking branch 'main/master' into check-native-library-…

94af3d7

…version-ECR-3148

vitvakatu approved these changes May 16, 2019

View reviewed changes

skletsun approved these changes May 16, 2019

View reviewed changes

dmitry-timofeev merged commit 77428d4 into exonum:master May 16, 2019

dmitry-timofeev deleted the check-native-library-version-ECR-3148 branch May 16, 2019 07:33


		private static final Logger logger = LogManager.getLogger(LibraryLoader.class);

		private static final LibraryLoader INSTANCE = new LibraryLoader(JAVA_BINDING_VERSION);

Check that the loaded native library is compatible [ECR-3148]: #882

Check that the loaded native library is compatible [ECR-3148]: #882

Uh oh!

Conversation

dmitry-timofeev commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bullet-tooth May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmitry-timofeev commented May 8, 2019 •

edited

Loading

bullet-tooth May 15, 2019 •

edited

Loading

coveralls commented May 8, 2019 •

edited

Loading