Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to enable build with JDK10 #2453

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@garyrussell
Copy link
Member

commented May 24, 2018

  • ambiguous .from() in IntegrationFlows
  • new Integer|Long(n) deprecated
  • Class.newInstance() deprecated
  • --add-modules where needed
  • illegal reflection
@artembilan
Copy link
Member

left a comment

The change is definitely good, but I left several doubts and questions.

Thanks

@@ -494,7 +495,7 @@ public Date foo() {

@Override
protected IntegrationFlowDefinition<?> buildFlow() {
return from(() -> new GenericMessage<>("flowAdapterMessage"),
return from((MessageSource<?>) () -> new GenericMessage<>("flowAdapterMessage"),

This comment has been minimized.

Copy link
@artembilan

artembilan May 24, 2018

Member

I wonder if that would better to rename to fromSupplier(). And yeah... remove that from(Supplier) altogether.

Breaking change, agreed, but no choice.

I will find this explicit casting style more annoying than removed method.

WDYT?

Show resolved Hide resolved ...springframework/integration/ip/tcp/connection/TcpNioConnectionTests.java
@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration
@SpringJUnitConfig
@EnabledOnJre(JRE.JAVA_8)

This comment has been minimized.

Copy link
@artembilan

artembilan May 24, 2018

Member

Maybe it's time to remove Eclipse Link tests altogether ?

* serialized {@link byte[]} to store {@link Message} to the MongoDB.
* And vice versa - to convert {@link byte[]} from the MongoDB to the {@link Message}.
* serialized {@code byte[]} to store {@link Message} to the MongoDB.
* And vice versa - to convert {@code byte[]} from the MongoDB to the {@link Message}.
* @author Artem Bilan
* @author Gary Russell
* @since 4.2.10
* @deprecated since 5.0 in favor of {@link MessageToBinaryConverter} and {@link BinaryToMessageConverter}

This comment has been minimized.

Copy link
@artembilan

artembilan May 24, 2018

Member

Maybe we can remove this class now?

I think that is a good catch for wrong JavaDocs - I don't see reason to add comment for that 😄

But thanks anyway!

@@ -123,6 +124,7 @@ public void withScriptVariableGenerator() throws Exception {
}

@Test
@EnabledOnJre(JRE.JAVA_8)

This comment has been minimized.

Copy link
@artembilan

artembilan May 24, 2018

Member

What's wrong here, please ?

This comment has been minimized.

Copy link
@garyrussell

garyrussell May 24, 2018

Author Member
java.lang.AssertionError
	at org.jruby.RubyModule.fetchConstant(RubyModule.java:4197)
	at org.jruby.RubyModule.getConstantAt(RubyModule.java:3681)
	at org.jruby.RubyModule.getConstantAt(RubyModule.java:3677)
	at org.jruby.javasupport.Java.getJavaPackageModule(Java.java:800)
	at org.jruby.javasupport.Java.getJavaPackageModule(Java.java:780)
	at org.jruby.javasupport.binding.ClassInitializer.initialize(ClassInitializer.java:47)
	at org.jruby.javasupport.binding.Initializer.setupProxyClass(Initializer.java:99)

I didn't feel like digging into JRuby

This comment has been minimized.

Copy link
@artembilan

artembilan May 24, 2018

Member

That's fine.
We may even remove all the Ruby tests.
I tried once to upgrade to the latest version and we have some conflicts with the Jython - they override some classes, but not fully 😢

I would say that JavaScript compatibility should be enough for us to trace: everything rest is just a target engine implementation and we are not responsible for their incompatibility with Java 10.

OK. Let's leave like you did. Maybe some comment on lines for the future consideration when Java 11 is going to be a base line ?

* JDK 10
* WARNING: An illegal reflective access operation has occurred
* WARNING: Illegal reflective access by org.springframework.util.ReflectionUtils
* to field com.sun.org.apache.xpath.internal.jaxp.XPathExpressionImpl.xpath

This comment has been minimized.

Copy link
@artembilan

artembilan May 24, 2018

Member

Interesting...
Maybe we can just end up with the "xpathExpression.expression path?

Anyway what is the premise of the error?
Is there any ideas ?

Thanks

This comment has been minimized.

Copy link
@garyrussell

garyrussell May 24, 2018

Author Member

In a future compiler, such illegal accesses will be denied.

This comment has been minimized.

Copy link
@artembilan

artembilan May 24, 2018

Member

In other words all our "Direct fields access" magic is going to be banned?

@ContextConfiguration
public class JaxbMarshallingIntegrationTests extends AbstractJUnit4SpringContextTests {
@SpringJUnitConfig
@EnabledOnJre(JRE.JAVA_8)

This comment has been minimized.

Copy link
@artembilan

artembilan May 24, 2018

Member

What here?

This comment has been minimized.

Copy link
@garyrussell

garyrussell May 24, 2018

Author Member

Needs jvmArgs --add-modules' 'java.xml.bind' for testRuntime and I don't know how to do that 😄

@garyrussell garyrussell force-pushed the garyrussell:jdk10 branch from cc74b7d to 2a0785f May 24, 2018

@artembilan

This comment has been minimized.

Copy link
Member

commented May 24, 2018

Something wrong with Groovy yet:

org.springframework.integration.groovy.config.GroovyServiceActivatorTests > invalidInlineScript() FAILED
    java.lang.AssertionError at GroovyServiceActivatorTests.java:201

Although this is not Java 10 compatibility issue...

@artembilan

This comment has been minimized.

Copy link
Member

commented Jun 1, 2018

There is something like this in Gradle logs:

> Task :spring-integration-xmpp:javadoc
javadoc: warning - You have not specified the version of HTML to use.
The default is currently HTML 4.01, but this will change to HTML5
in a future release. To suppress this warning, please specify the
version of HTML used in your documentation comments and to be
generated by this doclet, using the -html4 or -html5 options.

I see similar warn for all our modules.

Also all the SFTP tests fail for me on Windows:

java.lang.NoClassDefFoundError: Could not initialize class javax.crypto.JceSecurity

	at java.base/javax.crypto.JceSecurityManager.<clinit>(JceSecurityManager.java:65)
	at java.base/javax.crypto.Cipher.getConfiguredPermission(Cipher.java:2613)
	at java.base/javax.crypto.Cipher.getMaxAllowedKeyLength(Cipher.java:2635)
	at org.apache.sshd.common.cipher.Cipher.checkSupported(Cipher.java:83)
	at org.apache.sshd.common.cipher.BuiltinCiphers.<init>(BuiltinCiphers.java:102)
	at org.apache.sshd.common.cipher.BuiltinCiphers.<clinit>(BuiltinCiphers.java:54)
	at org.apache.sshd.common.BaseBuilder.<clinit>(BaseBuilder.java:71)
	at org.apache.sshd.server.SshServer.setUpDefaultServer(SshServer.java:412)
	at org.springframework.integration.sftp.SftpTestSupport.createServer(SftpTestSupport.java:61)

Maybe it requires some -add-modules to include as well?
No ideas so far. Also interesting how it works for you...

Thanks

@garyrussell

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2018

Yes, I noticed those javadoc warnings. Still TODO.

I did see a couple of SFTP failures on Mac OS. I don't recall if it was that error (but it looks familiar), but the last few times I had no problems, so I don't think it's as simple as needing add-modules; more of an intermittent problem; at least on Mac OS.

public void invalidInlineScript() throws Exception {
Message<?> message =
new ErrorMessage(new ReplyRequiredException(new GenericMessage<String>("test"), "reply required!"));
try {
this.invalidInlineScript.send(message);
fail("MessageHandlingException expected!");
}
catch (Exception e) {
catch (MessageHandlingException e) {

This comment has been minimized.

Copy link
@artembilan

artembilan Jun 2, 2018

Member

Gary, I have this test like:

	//INT-2399
	@Test
	public void invalidInlineScript() {
		Message<?> message =
				new ErrorMessage(new ReplyRequiredException(new GenericMessage<String>("test"), "reply required!"));
		try {
			this.invalidInlineScript.send(message);
			fail("MessageHandlingException expected!");
		}
		catch (MessageHandlingException e) {
			Throwable cause = e.getCause();
			assertEquals(ScriptCompilationException.class, cause.getClass());
			assertThat(cause.getMessage(),
					Matchers.containsString("No such property: ReplyRequiredException for class: script"));
		}
	}

And it works for me on both Java versions, as well as from IDE and Gradle.

Also this fixes the Travis failure.

@artembilan
Copy link
Member

left a comment

Unfortunately I can't find what is wrong with the SSL in case of SFTP module. Sounds like we need to escalate it up to the Apache sshd or even Java 10 😢

@@ -700,6 +711,14 @@ project('spring-integration-xml') {

testCompile "xmlunit:xmlunit:$xmlUnitVersion"
}

if (JavaVersion.current() != JavaVersion.VERSION_1_8) {
[compileTestJava]*.options*.compilerArgs = ['--add-modules', 'java.xml.bind']

This comment has been minimized.

Copy link
@artembilan

artembilan Jun 4, 2018

Member

You are missing in all these manipulation our global xLintArg option.
I wonder if we should move all the --add-modules to the top level config, along side with our [compileJava, compileTestJava]*.options*.compilerArgs = [xLintArg] there...

This comment has been minimized.

Copy link
@garyrussell

garyrussell Jun 4, 2018

Author Member

Perhaps += works here?

This comment has been minimized.

Copy link
@artembilan

artembilan Jun 4, 2018

Member

No, I tried 😢

garyrussell added some commits May 24, 2018

Changes to enable build with JDK10
- ambiguous `.from()` in `IntegrationFlows`
- new Integer|Long(n) deprecated
- `Class.newInstance()` deprecated
- --add-modules where needed
- illegal reflection

@garyrussell garyrussell force-pushed the garyrussell:jdk10 branch from 2a0785f to 3a1e349 Jun 11, 2018


}
if (JavaVersion.current() != JavaVersion.VERSION_1_8) {
[compileTestJava]*.options*.compilerArgs = ["$xLintArg", '--add-modules', 'java.xml.ws']

This comment has been minimized.

Copy link
@artembilan

artembilan Sep 19, 2018

Member

See some further instructions about new Java version dependencies configuration: https://spring.io/blog/2018/09/19/spring-web-services-3-0-4-2-4-3-released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.