Common Eclipse logging support

[fvgh]

With the integration of new Eclipse formatters, a common logging manager should be provided, supporting the following features:

• Manager shall be generic and suitable for forwarding log events to Gradle as well as Maven
• Manager shall handle the existing log calls of org.eclipse.core.runtime.Plugin (see greclipse for implementation details)
• Manager shall provide simple interface for own log events, like provided by Java Logger.
• Since some of the Eclipse formatters are less mature than others, the Manager shall be configurable whether to raise exceptions on certain log levels or not. Per default the Manager shall convert log events with a level of error or higher into run-time exceptions.

[fvgh]

@nedtwigg / @jbduncan : I would like to have your feedback before I continue... No hurry, whenever you find time...
Of-course feedback is welcome from anybody else as well.

I provided a POC for this enhancement.
The following is implemented (it's just a POC, there are still some things I need to clean-up):

1. Slf4J Eclipse logging service which maps Eclipse logging service to Slf4J.
2. Support in Eclipse JDT as an example
3. Local-First class loader which allows all extensions to access Gradle/Maven Slf4J logging interface.

I want logging in particular for Eclipse. Some Eclipse plugins already have and need a logging service. These individual implementations will be removed. Others (like JDT) had no service. In case no logging service is provided, Eclipse log messages are dropped, which I like to avoid.

The Local-First class loader would allow any class (extension or not) to use Slf4J. It has the benefit, that it has direct access to the log-level set by Maven and Gradle (tested with JUnit5 and Jung), and logs can be more gradual than std-err / std-out.

However, it is quite a big change to the framework, and I saw that e.g. for Eclipse log messages the std-err std-out is in the end sufficient. So if no one sees a benefit in using Slf4J, I would agree to drop it.
In Eclipse, Slf4J would stay if nobody disagrees, since I find System.err / System.out to restrictive.

@nedtwigg
For Fresh Mark, I needed to go back to the original class loader (without parent), since I got the following exception:

  Caused by: java.lang.RuntimeException: Error on line 13: jdk/nashorn/internal/runtime/ScriptObject
        at com.diffplug.freshmark.Parser$1ErrorFormatter.lambda$wrap$2(Parser.java:89)
        at com.diffplug.freshmark.ParserIntronExon.bodyAndTags(ParserIntronExon.java:71)
        at com.diffplug.freshmark.Parser.compile(Parser.java:162)
        at com.diffplug.freshmark.CommentScript.compile(CommentScript.java:85)
        ... 124 more
Caused by: java.lang.NoClassDefFoundError: jdk/nashorn/internal/runtime/ScriptObject
        at jdk.internal.dynalink.beans.StaticClassLinker$SingleClassStaticsLinker.createConstructorMethod(StaticClassLinker.java:138)
        at jdk.internal.dynalink.beans.StaticClassLinker$SingleClassStaticsLinker.<init>(StaticClassLinker.java:120)
        at jdk.internal.dynalink.beans.StaticClassLinker$1.computeValue(StaticClassLinker.java:108)
        at jdk.internal.dynalink.beans.StaticClassLinker$1.computeValue(StaticClassLinker.java:105)

Can you give me a hint, why that fails? Otherwise I'll investigate myself. First time I used my own class loader. Thought I understood the concept, but Fresh Mark proved me wrong. Regardless the outcome of this discussion, I would like to understand the problem. So if you can give me a nudge in the right direction, I would appreciate it.

[jbduncan]

Hi @fvgh. Thank you for asking me to have a look at this issue, but I fear I won't have the time and mental bandwidth to review it because I'm still getting adjusted to work and trying to tackle #277.

@nedtwigg, is this issue something you can review on your own? Or would you like me to have a look at it as well at some point?

[fvgh]

Hi @jbduncan, sorry, I should have formulated the request clearer. I don't want a code review, since it is just a POC. Since you guys worked a lot on Spotless, I wanted your opinion, whether Slf4J logging would be beneficial enough for all Spotless components, so that it justifies the overhead (local-first class loader). Or maybe you can see other use cases where you would profit from such a class loading concept. For Eclipse alone, it is not worth it.

[nedtwigg]

So let's say I have this (pseudo-script).

buildscriptClasspath {
   'spotless'
   'google-java-format:v1'
}

spotless {
  java {
    googleJavaFormat('v2')
  }
}

In this example, LocalClassLoader is going to use google-java-format:v1 - not v2. The trouble is it will get very difficult to debug issues, because Spotless' behavior will depend on what other libraries the user has loaded in their buildscript

If you want to pass some kind of logging classloader from the root classloader to the others, I think you'll have to enumerate exactly which packages you need to load, along these lines:

https://github.com/mytakedotorg/mytakedotorg/blob/342779de86bfe7a116c3a1b9aa7d9c9cf08fd829/server/src/test/java/common/DevHotReload.java#L109-L137

I personally have better luck stepping through a debugger than figuring out how to enable the log-level that I need, but I'm all for people using whichever tool they prefer. If we can figure out a way to accomplish this without adding too much code into spotless-lib I'm all for merging it in.

[fvgh]

@nedtwigg Thanks for the link. I had something similar in mind. What is vexing me is the fact that the Fresh Mark worked before. All the Local-First class loader does (well, I hope that I got that right) is: "If you don't find the class in my URLs, ask the parent (so the class loader Gradle/Maven uses for Spotless plugin)." I don't know how class loader from Gradle/Maven behave, but I thought that is black spot is OK.
Before, it should have been working (parent was null) like: "Look in boostrap and extensions. If you don't find the class there, search my URLs."
Is my understanding correct so far?

In that case, I find it still suspicious, that Fresh Mark fails. Spotless should provide all Fresh Mark it needs in the URLs. So FreshMark should never bother the Gradle/Maven class loader (except of course for java/javax). Or is this assumption wrong?

[nedtwigg]

Spotless should provide all Fresh Mark it needs in the URLs

Not quite, the bootstrap classloader (Object.class, Class.class, jdk/nashorn/internal/runtime/ScriptObject.class, etc.) will be needed too. Classloaders are really difficult. Might wanna take a look at this: https://stackoverflow.com/questions/5445511/how-do-i-create-a-parent-last-child-first-classloader-in-java-or-how-to-overr

[fvgh]

@nedtwigg
I replaced "local first" class loading approach by "missing only" one.

After looking at JVMS §5.3, I hope I understand now, why for example the getResource(s) methods do not require an overload, and overloading findClass is sufficient. Since the class already has a defining class loader, that one is used.

In opposite to the "parent last" class loader implementation you mentioned, I see no harm to derive from URLClassLoader, do you?

You mentioned also the possibility to enumerate SLF4J packages. I don't like this approach since:

• I must make assumptions which package names are provided by SLF4J
• I would either provide//download SLF4J JARs with the features, I don't want to use, or I have a redundancy of information when specifying the dependency scope and enumerating the packages.

Hence I prefer to stick with explicitly reduce the dependency scope.

When it comes to naming the classes, I thought it is more readable to mention the purpose (loading features), instead of the current implementation (load missing classes from build tool class loader).

Please let me know what you think.

[nedtwigg]

Assuming it works, I like the design of your FeatureClassLoader. However, I would still limit its packages to slf4j, as such: bbdc876

I must make assumptions which package names are provided by SLF4J

The entire point of SLF4J is to be a stable set of interfaces that can be implemented by any jar, thus we don't need to worry about them changing

I would either provide//download SLF4J JARs with the features

See my linked commit above, I don't think this is true

The trouble is it will get very difficult to debug issues, because Spotless' behavior will depend on what other libraries the user has loaded in their buildscript

It's common to use Class.forName(blah) to test what features are available on the classpath. Unless we limit the hole to slf4j, then any formatter might resolve a random class from the buildscript classloader. If the user changes their buildscript in ways that shouldn't affect spotless, it might affect Spotless behavior in a way that is very hard to debug.

[fvgh]

Assuming it works...

Sure.

The entire point of SLF4J is to be a stable set of interfaces that can be implemented by any jar, thus we don't need to worry about them changing

Not sure about that. What worries me is the following part of the SLF4J compatibility statement:

Mixing different versions of slf4j-api.jar and SLF4J binding can cause problems. For example, if you are using slf4j-api-1.8.0-beta2.jar, then you should also use slf4j-simple-1.8.0-beta2.jar, using slf4j-simple-1.5.5.jar will not work.

So in my opinion I cannot make a dependency to slf4j-api in the Eclipse-Base (feature) and expect that the binding used by Maven and Gradle (build tools) will work. I must ensure that the slf4j-api classes of build tools are used by the feature classes. Hence I thought that it would be funny to declare the slf4j-api as a run-time dependency.

I understand your point about checking of available features at run-time. But in this case maybe it is cleaner to omit the dependency scope reduction on the feature, but assure that the class loader always select the build-tool class loader for org.slf4j.* classes.

[nedtwigg]

From that same compatibility statement:

from the client's perspective all versions of slf4j-api are compatible. Client code compiled with slf4j-api-N.jar will run perfectly fine with slf4j-api-M.jar for any N and M. You only need to ensure that the version of your binding matches that of the slf4j-api.jar. You do not have to worry about the version of slf4j-api.jar used by a given dependency in your project. You can always use any version of slf4j-api.jar, and as long as the version of slf4j-api.jar and its binding match, you should be fine.

I thought the whole point of punching through to the buildscript classpath was to get SLF4J. If that isn't the point, what is?

[fvgh]

Maybe I misinterpreted your previous statement

The entire point of SLF4J is to be a stable set of interfaces that can be implemented by any jar

Yes, the slf4j-api.jar is stable, so the feature can compile against version N and use later on M. But the feature needs a binding. That's provided by the build tool. So the FeatureClassLoader needs to assure that the binding classes, which are missing in the feature, are taken from the build tool.

But the SLF4J compatibility statement states that the feature can't use binding M with API N. As a solution, the feature dependency scope for slf4j-api.jar is reduced to compile time, to ensure that API and binding are both not part of the feature itself.

With my current solution, the FeatureClassLoader in the spotless-lib and the Eclipse feature build script, which reduces the compile scope of the slf4j-api.jar, are independent. The FeatureClassLoader does not know anything about SLF4J. I just want to keep this independence.

I understand your remark, why my current solution lead to unexpected results (feature testing).
Your proposal (bbdc876) has the drawback that it implies a contract between FeatureClassLoader and the feature dependency scope.

So my proposal would be something like this:

protected Class<?> findClass(String name) throws ClassNotFoundException {
  if (name.startsWith("org.slf4j.")) {
    return buildToolClassLoader.loadClass(name);
  } else {
   return super.findClass(name);
  }

Then the feature does not need to take care of any SLF4J specific dependency scope reductions.
Performance will be not as good as in your solution. Furthermore the slf4j-api.jar (~25kb) version which is part of the feature dependency will be downloaded, though it would never be used.
But since we would have less code/build-script lines (as in my current solution) and there is no contract between feature and lib implementation, I think it is worth it.

OK for you?

[nedtwigg]

Ranked priorities. If we have to make a sacrifice, we should sacrifice 3 rather than 1 or 2.

1. Keep the formatter classloaders isolated from the buildscript
2. Enable logging
3. Keep the formatter classloader general purpose, with no specific carveouts

my proposal would be something like this...

Looks good to me, if it works ;-) I think it will.

Then the feature does not need to take care of any SLF4J specific dependency scope reductions.
Performance will be not as good as in your solution. Furthermore the slf4j-api.jar (~25kb) version which is part of the feature dependency will be downloaded, though it would never be used.
But since we would have less code/build-script lines (as in my current solution) and there is no contract between feature and lib implementation, I think it is worth it.

I don't follow, but that's okay. I also think this is likely to affect formatters besides just the eclipse one. I would be surprised if slf4j wasn't supported by some of our formatters already.

e.g. the currently open PR #328 https://github.com/antlr/Antlr4Formatter/search?q=slf4j&unscoped_q=slf4j

[nedtwigg]

Published in 1.18.0

[fvgh]

@nedtwigg I did not delete eclipse_base_logger_limited branch since it is yours. I don't need it anymore. Do you?

[nedtwigg]

I don't, feel free to delete :)