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

Move to JLine 3.x to avoid Jansi 1.x dependency/usage #19510

Merged
merged 1 commit into from
Aug 21, 2021
Merged

Move to JLine 3.x to avoid Jansi 1.x dependency/usage #19510

merged 1 commit into from
Aug 21, 2021

Conversation

jaikiran
Copy link
Member

Potential fix for #19491

As noted in that discussion, the JLine 2.x is EOLed and its usage it leading to issues described in that bug.

I've done absolutely zero testing of this change. I have only ensured that the whole build compiles and builds fine. I want to make sure there are no basic issues with this change and see how CI reacts. Tomorrow, I will try and get hold of the Windows box again and see if this fixes that original issue.

In the meantime, there are a couple of things to sort out:

  • I see there's a CreateProjectMojoIT testcase in the project. I wonder why it didn't catch this issue on our Windows CI setup
  • I see that @gastaldi noted in the issue that he might have a way to get rid of the JLine usage in this code altogether, which I think would be the best/ideal thing. I have no historical knowledge of this code, so if that's possible, then I will close this PR and let's go with @gastaldi's approach. One less library dependency to maintain.

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven area/platform Issues related to definition and interaction with Quarkus Platform labels Aug 19, 2021
@gsmet
Copy link
Member

gsmet commented Aug 19, 2021

There's a good chance we will want to backport this so I would be in favor of not making too big changes here (and maybe have a followup PR later to remove JLine support). I have no idea how to test that this works though. I'll let @gastaldi chime in.

@jaikiran
Copy link
Member Author

I have no idea how to test that this works though

This Prompter gets used only in 2 Mojos (the CreateProjectMojo and CreateExtensionMojo), so I will run a basic manual test on the Windows box once I get access to it tomorrow.

I'll also check tomorrow why our automated test(s) didn't catch this.

@gastaldi
Copy link
Contributor

I'm building this PR in my Windows VirtualBox and will let you know

@jaikiran
Copy link
Member Author

I'll also check tomorrow why our automated test(s) didn't catch this.

Ha! Turns out our CI (rightly) uses Maven 3.8.1[1]. I just downloaded this version to do a basic check and turns out 3.8.1 version of Maven ships with jansi-1.71.1 of Jansi, which is why I believe we didn't see this issue in our CI. It looks like 3.8.2 of Maven brought in this new jansi-2.x version. Release notes don't show anything related to this https://maven.apache.org/docs/3.8.2/release-notes.html. I will have to dig more into their complete changes to see if this is intentional upgrade.

[1] https://github.com/quarkusio/quarkus/blob/main/.mvn/wrapper/maven-wrapper.properties#L1

@gastaldi
Copy link
Contributor

Release notes don't show anything related to this

@jaikiran looks like it's only mentioned in JIRA: https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12316922&version=12349965

@jaikiran
Copy link
Member Author

Yes, it looks like it first got upgrade in Maven 4.x and then was backported into 3.8.2 for this https://issues.apache.org/jira/browse/MNG-6239.

It's probably not a bad thing though. This gives us a chance to move away from JLine 2.x.

@gastaldi
Copy link
Contributor

I can confirm this works:

PS C:\temp> mvn io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:create `
>> -DprojectGroupId="com.infosupport.pitstop.customermanagement" `
>> -DprojectArtifactId="customer-management" `
>> -DclassName="com.infosupport.pitstop.customermanagement.CustomersResource" `
>> -Dpath="customer-management"clear
[INFO] Scanning for projects...
[INFO]
[INFO] ------------------< org.apache.maven:standalone-pom >-------------------
[INFO] Building Maven Stub Project (No POM) 1
[INFO] --------------------------------[ pom ]---------------------------------
[INFO]
[INFO] --- quarkus-maven-plugin:999-SNAPSHOT:create (default-cli) @ standalone-pom ---
ago 19, 2021 12:15:07 PM org.jline.utils.Log logr
WARNING: Unable to create a system terminal, creating a dumb terminal (enable debug logging for more information)
[INFO] -----------
[INFO]
applying codestarts...
[INFO] >> java
>> maven
>> quarkus
>> config-properties
>> dockerfiles
>> maven-wrapper
>> resteasy-codestart
[INFO]
-----------
[SUCCESS] quarkus project has been successfully generated in:
--> C:\temp\customer-management
-----------
[INFO]
[INFO] ========================================================================================
[INFO] Your new application has been created in C:\temp\customer-management
[INFO] Navigate into this directory and launch your application with mvn quarkus:dev
[INFO] Your application will be accessible on http://localhost:8080
[INFO] ========================================================================================
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  3.297 s
[INFO] Finished at: 2021-08-19T12:15:08-03:00
[INFO] ------------------------------------------------------------------------

@jaikiran
Copy link
Member Author

I can confirm this works:

Excellent. Thank you for running the tests with this PR. I'll let the GitHub action jobs complete and in a separate personal branch of mine will trigger a GitHub job by upgrading to Maven 3.8.2 to see if there are any other issues that we need to be aware of.

@gastaldi
Copy link
Contributor

gastaldi commented Aug 19, 2021

Not a big deal, but I wonder if we can somehow get rid of the warning:

ago 19, 2021 12:15:07 PM org.jline.utils.Log logr
WARNING: Unable to create a system terminal, creating a dumb terminal (enable debug logging for more information)

@gastaldi gastaldi linked an issue Aug 19, 2021 that may be closed by this pull request
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 19, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 54690ea

Status Name Step Test failures Logs Raw logs
Gradle Tests - JDK 11 Windows Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

io.quarkus.gradle.devmode.InjectQuarkusAppPropertiesDevModeTest.main line 19 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:166)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:939)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:908)
	at io.quarkus.test.devmode.util.DevModeTestUtils.getHttpResponse(DevModeTestUtils.java:130)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpResponse(QuarkusDevGradleTestBase.java:126)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpRes...

io.quarkus.gradle.devmode.ModuleWithParentDependencyDevModeTest.main line 14 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:166)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:939)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:908)
	at io.quarkus.test.devmode.util.DevModeTestUtils.getHttpResponse(DevModeTestUtils.java:130)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpResponse(QuarkusDevGradleTestBase.java:126)
	at io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpRes...

@famod
Copy link
Member

famod commented Aug 19, 2021

FWIW, I'm working on updating to Maven 3.8.2. This is currently waiting for the Upgrade to Gradle 7.2 (sic!).

Btw, we could add jline:jline to bannedDependencies.

@jaikiran
Copy link
Member Author

The failures in this PR look unrelated.

As for adding jline:jline as a banned dependency, is that a common thing we do for outdated libraries? Given that the new version of JLine that we are using in this PR uses a completely different package namespace, is banning necessary?

Hello @famod, I'll let you continue with the 3.8.2 work. I was merely interested in a complete run of this PR against 3.8.2, but it doesn't look straightforward given certain other unrelated dependency issues.

@gastaldi
Copy link
Contributor

It makes sense to ban it because it is incompatible with Maven 3.8.2, so +1 to that

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file labels Aug 20, 2021
@jaikiran
Copy link
Member Author

Done. I've updated the PR to ban jline:jline

@gastaldi gastaldi added triage/backport? triage/waiting-for-ci Ready to merge when CI successfully finishes labels Aug 20, 2021
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this but I really think that we should try to get rid of the warning WARNING: Unable to create a system terminal, creating a dumb terminal (enable debug logging for more information). We fight hard to get our output as clean as possible.

@jaikiran
Copy link
Member Author

Thanks for this but I really think that we should try to get rid of the warning WARNING: Unable to create a system terminal, creating a dumb terminal (enable debug logging for more information). We fight hard to get our output as clean as possible.

I'll take a look what this warning is about.

@jaikiran jaikiran removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 21, 2021
@jaikiran
Copy link
Member Author

@gsmet, Done. I've now updated this PR with a change that should prevent that warning message.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 21, 2021

Failing Jobs - Building 241d5b0

Status Name Step Test failures Logs Raw logs
MicroProfile TCKs Tests Verify Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-config

org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesDefaultOnBean line 172 - More details - Source on GitHub

java.lang.NullPointerException
	at org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesDefaultOnBean(ConfigPropertiesTest.java:172)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
	at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
	at jdk.internal.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.im...

org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesNoPrefixOnBean line 149 - More details - Source on GitHub

java.lang.NullPointerException
	at org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesNoPrefixOnBean(ConfigPropertiesTest.java:149)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
	at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
	at jdk.internal.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.i...

org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesNoPrefixOnBeanThenSupplyPrefix line 156 - More details - Source on GitHub

java.lang.NullPointerException
	at org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesNoPrefixOnBeanThenSupplyPrefix(ConfigPropertiesTest.java:156)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
	at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
	at jdk.internal.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.a...

org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesPlainInjection line 106 - More details - Source on GitHub

java.lang.NullPointerException
	at org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesPlainInjection(ConfigPropertiesTest.java:106)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
	at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
	at jdk.internal.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.i...

org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesWithPrefix line 115 - More details - Source on GitHub

java.lang.NullPointerException
	at org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesWithPrefix(ConfigPropertiesTest.java:115)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
	at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
	at jdk.internal.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.impl....

org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesWithoutPrefix line 132 - More details - Source on GitHub

java.lang.NullPointerException
	at org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesWithoutPrefix(ConfigPropertiesTest.java:132)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
	at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
	at jdk.internal.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian.core.im...

org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testNoConfigPropertiesAnnotationInjection line 165 - More details - Source on GitHub

java.lang.NullPointerException
	at org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testNoConfigPropertiesAnnotationInjection(ConfigPropertiesTest.java:165)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
	at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
	at jdk.internal.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.jboss.arquillian...

@jaikiran
Copy link
Member Author

jaikiran commented Aug 21, 2021

These latest Microprofile config TCK job failures with NullPointerException is surprising. My guess is they are not related to the change in this PR. Have these failed before with this exception? I can trigger a fresh run for this PR to see if they go away but I don't want to lose the logs and context from this current failed run, so just checking.

@gsmet
Copy link
Member

gsmet commented Aug 21, 2021

Yeah, I have seen them failing many times. /cc @radcortez

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks.

@gsmet gsmet merged commit adf5989 into quarkusio:main Aug 21, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Aug 21, 2021
@jaikiran jaikiran deleted the 19491 branch August 21, 2021 13:53
@gsmet gsmet modified the milestones: 2.3 - main, 2.1.4.Final Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven area/platform Issues related to definition and interaction with Quarkus Platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash during mvn io.quarkus-maven-plugin:2.1.2.Final:create
4 participants