google / flogger

A Fluent Logging API for Java
Apache License 2.0
1.45k stars 123 forks source link

JPMS: Maven artifacts all in the same java package cause issues #22

Open forax opened 6 years ago

forax commented 6 years ago

The maven artifacts all use the same java package (com.google.common.flogger) but are separate jars, which is a problem in Java 9 with the Module system enabled (JPMS).

cgruber commented 6 years ago

Do we have any ideas around this? Java modules break a fairly widely used pattern at google, using package-private to allow implementation classes to talk to each other (or tests), while keeping consumers from depending on details. I'm not sure how to resolve this sort of issue without changing that practice in Google's open-source stuff, at least.

The nice thing is that we have bazel visibility as a different way to protect access (mark an impl package as only visible to a package which materializes it for consumers, plus the tests). But it still means moving away from using package-friendly access. :/

forax commented 6 years ago

A Java module can export a package to only a predefined list of modules [1]. This used internally in the JDK to allow some modules to access to jdk.internal.misc.Unsafe without granting any unknown modules an access to it.

[1] http://openjdk.java.net/jeps/261

hagbard commented 6 years ago

@forax Are you saying that there's a way we can resolve this without changing package structure (since changing package structure is going to be incredibly difficult) ?

forax commented 6 years ago

It depends what you mean by changing the package structure, the main issue if that the default backend jar comes with classes defined com.google.common.flogger which is a big No for JPMS because another jar (the API jar) also comes with classes defined in com.google.common.flogger.

If the API par is defined in com.google.common.flogger and the backend par is defined in com.google.common.flogger.backend then there is no issue.

hagbard commented 6 years ago

Yes, that's what I mean by "changing the backend structure". Refactoring to move classes to alternate packages is a very non-trivial task unfortunately. It's doable, but needs to be justified in terms of effort.

Some of the backend classes are deliberately in the same package to allow package access to be used on APIs which we don't want to be public, so changing packages means opening up the internals in ways which aren't really desirable.

However the alternative idea would be to merge the frontend and backend jars into one jar/artifact (it's primarily two build targets because of how dependencies inside Google work. If we can merge it during the build/packaging process that might help.

Can you tell me the precisely what you mean by ""which is a problem in Java 9 with the Module system enabled (JPMS)"".

forax commented 6 years ago

so changing packages means opening up the internals in ways which aren't really desirable.

No, because you have a finer control of which module access on which package if you define a module-info

If we can merge it during the build/packaging process that might help.

It's basically what i'm doing, just after having dowloaded the flogger jars from Maven, i merge them, it works because i'm using my own build tool, as far as i know, Maven or Gradle doesn't support this kind of things. see https://github.com/forax/beautiful_logger/blob/master/build.pro#L44

Can you tell me the precisely what you mean by ""which is a problem in Java 9 with the Module system enabled (JPMS)"".

If you put the two jars in the module-path the VM refuse to start.

Can users work around this on their end (e.g. JDK flags for specifying non compliant packages) ?

yes, do not put the jars in the module-path, keep them in the classpath, which means you can not use a tool like jlink that requires to have all jars in the module-path

What's the requirement for satisfying the JPMS (is it more complex than just keeping packages defined in only a single jar) ? apart not having split-packages, the other requirements are

  • do not use JDK internals
  • provides a module-info.java (1) or at least a Autonatic-Module-Name entry in the manifest

Does anyone know if users will already be faced with many common libraries that don't comply with this requirement (if Flogger is one of hundreds of examples, I'm less inclined to worry immediately).

Spring 5, Netty or Groovy 3 have been modified to comply, Maven not yet.

(1) here is the one i use, obviously, you may not want to export all packages (or define the module as open)

open module com.google.common.flogger {
  requires java.base;
  requires java.logging;
  requires javax.annotation;
  exports com.google.common.flogger.backend.system;
  exports com.google.common.flogger.parameter;
  exports com.google.common.flogger.backend;
  exports com.google.common.flogger;
  exports com.google.common.flogger.util;
  exports com.google.common.flogger.parser;
}
hagbard commented 6 years ago

Interesting. Thanks for the info. I'll open an internal bug with this information in to track things and look at the work involved in each of the steps. I can't promise it'll happen soon (I've not got a lot of time to work on things at the moment) but you've convinced me it's worth doing.

You say ""you have a finer control of which module access on which package if you define a module-info"", but that's only for Java 9 users I assume (and only if they use the module system?)

I'm interested to see that you didn't run into issues with "do not use JDK internals" given the use of sun.misc.unsafe for fast stack trace stuff.

forax commented 6 years ago

yes, only for java 9+ users and only if the jars are in the module-path.

sun.misc.Unsafe has a special treatment because the class is useful (even if its methods are unsafe), widely used (Android has it's own version), and there are no safe replacement for all the methods. So sun.misc.Unsafe still exist in the modular world (in the module jdk.unsupported), all methods that have a public replacement are deprecated and later removed, all other methods are kept in place until there is a replacement. New API like java.lang.StackWalker, java.lang.invoke.MethodHandles.Lookup.privateLookupIn|defineClass, java.lang.invoke.VarHandle have been introduced as replacement for stack walking, accessing private members, dynamically defining a class or fast access to fields/array/byte buffers using different memory mode.