-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix dev mode launch on Windows, replace jline with Aesh in the quarkus-maven-plugin #19760
Fix dev mode launch on Windows, replace jline with Aesh in the quarkus-maven-plugin #19760
Conversation
read(connection, ReadlineBuilder.builder().enableHistory(false).build(), prompts.iterator()); | ||
connection.openBlocking(); | ||
} finally { | ||
connection.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartwdouglas quick question. I've switched the Maven plugin's prompter from jline to Aesh (since it's already on the classpath anyway). I was wondering whether i should restore the terminal's state like you do in the DevMojo (https://github.com/quarkusio/quarkus/blob/main/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java#L445) or is it ok to simply close the connection. I am not doing anything fancy here, simply collecting the input. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you should be ok. I put the terminal in raw mode so if the process crashes I need to restore it to the original state.
bcdb10c
to
0cfb90e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Funny, I thought Aesh was permanently removed from Quarkus, looks like I was wrong 😛 |
<configuration> | ||
<artifactSet> | ||
<includes> | ||
<include>org.fusesource.jansi:jansi</include> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work with Maven 3.8.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knutwannheden just confirmed to me it works for both 3.8.1 and 3.8.2.
This change actually reduces the number of Jansi versions on the classpath by removing JLine which shades a Jansi version. With change we shade the Jansi version which is already on the classpath. Why exactly we have to do this to make it work is mystery to me though.
Fixes #19673
quarkus-maven-plugin
(Aesh is already present on the classpath as thequarkus-core-deployment
dependency)quarkus-maven-plugin
(to fix classloading on Windows)Prompter
a regular class, not a MavenComponent