JCLOUDS-1333: JCLOUDS-1334: JCLOUDS-1470: Require Java 8 and Guava 22 #75

gaul · 2020-06-14T03:11:07Z

This allows compatibility with Guava 29. Also unwind some older
workarounds.

gaul · 2020-06-14T03:12:55Z

This will likely fail in CI due to configuration but locally tests fail due a gson issue. I don't understand our tower of hacks. @nacx Any suggestions on how to address this?

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.17:test (default-test) on project jclouds-core: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:2.17:test failed: There was an error in the forked process
[ERROR] java.lang.NoClassDefFoundError: com/google/gson/TypeAdapterFactory
[ERROR]         at java.lang.ClassLoader.defineClass1(Native Method)
[ERROR]         at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
[ERROR]         at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
...
[ERROR] Caused by: java.lang.ClassNotFoundException: com.google.gson.TypeAdapterFactory
[ERROR]         at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
[ERROR]         at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
[ERROR]         at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
[ERROR]         at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
[ERROR]         ... 39 more

gaul · 2020-06-14T06:39:59Z

This is strange, mvn test cannot find gson but mvn install makes progress until failing tests.

gaul · 2020-06-14T07:15:18Z

Also need to use an older JDK to work around weak 1024-bit SSL certificates. I found that OrganizationApiExpectTest.testDeleteGroupFailsOn404 fails due to special handling in ChefErrorHandler.handleError. Commenting this out allows the test to succeed but I don't understand how it passed before.

gaul · 2020-06-14T07:30:47Z

apis/chef/src/main/java/org/jclouds/chef/handlers/ChefErrorHandler.java

-               if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
-                  exception = new ResourceNotFoundException(message, exception);
-               }
+               exception = new ResourceNotFoundException(message, exception);


@nacx Any comment on this change?

This is a common pattern in jclouds APIs: if you attempt to delete a resource that does not exist, do not fail the operation because you're already in the desired state.
Why does this need to be changed?

Oh, I see that it is because of a test. I'd say the test is wrong, not this, so I'd just change the test and keep the current behavior.

gaul · 2020-06-14T07:38:02Z

project/pom.xml

    <maven.compile.deprecation>true</maven.compile.deprecation>
    <maven.site.url.base>gitsite:git@github.com/jclouds/jclouds-maven-site.git</maven.site.url.base>
-    <guava.version>18.0</guava.version>
+    <guava.version>22.0</guava.version>


We could support 21.0 with some workarounds for SimpleTimeLimiter although this doesn't seem worth the effort.

nacx · 2020-06-15T19:14:55Z

There were some gson hacks we did to be able to make it OSGi friendly, as there are uses of an internal gson package we use that has been unexpected in newer versions of the library. That made it impossible to upgrade to newer versions of it.

Basically, we shaded those packages and renamed them so we can safely use them in our codebase.

Now we're in a different place, and I think we should revisit our OSGi support. We moved jclouds-Karaf to the Karaf project because we lack the expertise in OSGi and maintaining it was adding too much burden on us. Does the same apply here? Should we just undo this hack, go with standard gson and upgrade normally, and remove the OSGi compatibility from our list of responsibilities? This shading hack is something that can be done by users, or even by the Karaf project, probably in a better way, so I'd be in favour of undoing this at the expense of losing compatibility. The cost of maintaining this is too high for the current community.

nacx · 2020-06-15T19:19:51Z

Also need to use an older JDK to work around weak 1024-bit SSL certificates.

Is this because of the certificates we use in tests? Or something spotted running live tests, or because how we configure the SSL module by default? It would be great if we could upgrade and not be tied to older JDKs.

gurkerl83 · 2020-06-15T22:39:13Z

Hello, we use your JClouds in our OSGi project. Actually OSGi and all the regulations around packet exports is a difficult topic. Especially when an OSGi compatibility is set by a library and others still access restricted areas of the library.

I can't understand why the makers of JSON restrict these sections, instead every package of the library should be exported, after all there is not much to hide

internal/
internal/bind
internal/bind/utile
internal/reflect

I'm not sure if GSON really intends to keep these packages away from the outside world, or if this is just a misconfiguration. You can try submitting a merge request to GSON by exporting all packages, adding four lines to the "exportcontents" section of the bnd.bnd configuration file Due to other experiences with JSON I can't promise if a change of this kind will be accepted by the developers. After all, very few Java developers know about OSGi and the rules of the construct, developers working at Google are no exception.

The proposal to let the user override the GSON module is basically good. The way of proceeding is left to him, but it should be comprehensible, the requirement to do so should be documented in JClouds in any case. My approach at this point is to create a new git project, integrate a git submodule into GSON, and use a script (changing the BND file before build) or a Maven plugin to influence the build process of GSON. An example of where we do this is https://github.com/project-millipede/neo4j-ogm-osgi. Since GSON releases are not released too often, updating the submodule is not a hassle.

I am interested in the process, whatever this will look like, I am curious.

nacx · 2020-06-15T23:02:33Z

I'm not sure if GSON really intends to keep these packages away from the outside world, or if this is just a misconfiguration. You can try submitting a merge request to GSON by exporting all packages, adding four lines to the "exportcontents" section of the bnd.bnd configuration file Due to other experiences with JSON I can't promise if a change of this kind will be accepted by the developers. After all, very few Java developers know about OSGi and the rules of the construct, developers working at Google are no exception.

We tried, but unfortunately, it was rejected: google/gson#916

The main issue here is that we can no longer afford to keep the OSGi compatibility because of the lack of expertise in the active contributor group to the project. That's why we offloaded the Karaf support to the Karaf project, and this is a remaining piece that is preventing us from modernizing the codebase. Help there is really welcome.

Ideally, we should try to refactor the jclouds core to stop relying on that, or even copy the files to the jclouds codebase and keep them there, but I think we can't afford the overhead of keeping OSGi into account when the expertise is not here.

gaul · 2020-06-16T13:38:42Z

Is this because of the certificates we use in tests? Or something spotted running live tests, or because how we configure the SSL module by default? It would be great if we could upgrade and not be tied to older JDKs.

#71 should resolve the certificate length but we need to upgrade BouncyCastle first. I don't remember exactly but that was blocked on another dependency. Working through these...

gaul · 2020-06-16T13:40:14Z

Hello, we use your JClouds in our OSGi project. Actually OSGi and all the regulations around packet exports is a difficult topic. Especially when an OSGi compatibility is set by a library and others still access restricted areas of the library.

@gurkerl83 thanks for chiming in. We lack OSGi expertise and would appreciate if you could submit any improvements. I am trying to modernize the dependencies and fear that this will break things I cannot test like OSGi.

This allows compatibility with Guava 29. Also unwind some older workarounds.

gurkerl83 · 2020-06-24T10:23:27Z

Hi, I have create a follow up JIRA ticket to summarise the GSON problematic https://issues.apache.org/jira/browse/JCLOUDS-1548. Further I wanted to know when the current merge request will be merged. I have also reviewed the changes, which in my opinion are legit, also all tests pass.
Thx!

gaul · 2020-06-24T14:07:00Z

@nacx Any further comments?

nacx · 2020-06-24T19:38:59Z

Nope, LGTM. Thanks!

cdancy · 2020-08-27T14:13:15Z

@gaul @nacx any chance we can get a new release with this fix?

gaul · 2020-08-27T23:15:00Z

Probably a few months. We are still working on the OkHttp and gson dependencies.

gaul requested a review from nacx June 14, 2020 03:11

gaul commented Jun 14, 2020

View reviewed changes

gaul force-pushed the guava-22 branch 2 times, most recently from ed06a3b to 3405e43 Compare June 14, 2020 07:59

JCLOUDS-1333: JCLOUDS-1334: JCLOUDS-1470: Require Java 8 and Guava 22

4e129f8

This allows compatibility with Guava 29. Also unwind some older workarounds.

gaul force-pushed the guava-22 branch from 3405e43 to 4e129f8 Compare June 16, 2020 13:41

remk mentioned this pull request Jun 17, 2020

James 3223 - bump dependency version to fix vulnerabilities linagora/james-project#3445

Closed

nacx approved these changes Jun 24, 2020

View reviewed changes

gaul merged commit 62767a1 into apache:master Jun 24, 2020

gaul deleted the guava-22 branch June 24, 2020 23:14

gaul mentioned this pull request Jun 25, 2020

JCLOUDS-1333: Require JDK 1.8 jclouds/jclouds#1197

Closed

cdancy mentioned this pull request Aug 27, 2020

jclouds.proxy-host not working cdancy/jenkins-rest#119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JCLOUDS-1333: JCLOUDS-1334: JCLOUDS-1470: Require Java 8 and Guava 22 #75

JCLOUDS-1333: JCLOUDS-1334: JCLOUDS-1470: Require Java 8 and Guava 22 #75

gaul commented Jun 14, 2020

gaul commented Jun 14, 2020

gaul commented Jun 14, 2020

gaul commented Jun 14, 2020

gaul Jun 14, 2020

nacx Jun 15, 2020

nacx Jun 15, 2020

gaul Jun 16, 2020

gaul Jun 14, 2020

nacx Jun 15, 2020

nacx commented Jun 15, 2020

nacx commented Jun 15, 2020

gurkerl83 commented Jun 15, 2020

nacx commented Jun 15, 2020 •

edited

gaul commented Jun 16, 2020

gaul commented Jun 16, 2020

gurkerl83 commented Jun 24, 2020

gaul commented Jun 24, 2020

nacx commented Jun 24, 2020

cdancy commented Aug 27, 2020

gaul commented Aug 27, 2020

JCLOUDS-1333: JCLOUDS-1334: JCLOUDS-1470: Require Java 8 and Guava 22 #75

JCLOUDS-1333: JCLOUDS-1334: JCLOUDS-1470: Require Java 8 and Guava 22 #75

Conversation

gaul commented Jun 14, 2020

gaul commented Jun 14, 2020

gaul commented Jun 14, 2020

gaul commented Jun 14, 2020

gaul Jun 14, 2020

Choose a reason for hiding this comment

nacx Jun 15, 2020

Choose a reason for hiding this comment

nacx Jun 15, 2020

Choose a reason for hiding this comment

gaul Jun 16, 2020

Choose a reason for hiding this comment

gaul Jun 14, 2020

Choose a reason for hiding this comment

nacx Jun 15, 2020

Choose a reason for hiding this comment

nacx commented Jun 15, 2020

nacx commented Jun 15, 2020

gurkerl83 commented Jun 15, 2020

nacx commented Jun 15, 2020 • edited

gaul commented Jun 16, 2020

gaul commented Jun 16, 2020

gurkerl83 commented Jun 24, 2020

gaul commented Jun 24, 2020

nacx commented Jun 24, 2020

cdancy commented Aug 27, 2020

gaul commented Aug 27, 2020

nacx commented Jun 15, 2020 •

edited