You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2025/02/03 12:26:42 UTC
[PR] [MNG-8535] Full std stream support for executor [maven]
cstamas opened a new pull request, #2082:
URL: https://github.com/apache/maven/pull/2082
The std stream support was incomplete in maven-executor
---
https://issues.apache.org/jira/browse/MNG-8535
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8535] Full std stream support for executor [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2082:
URL: https://github.com/apache/maven/pull/2082#discussion_r1939321129
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -305,16 +307,29 @@ protected BuildEventListener doDetermineBuildEventListener(C context) {
}
protected void createTerminal(C context) {
+ boolean stdStreamsRedirected = System.console() == null
Review Comment:
I don't think those boolean are needed.
`System.console()` kinda delegates to JDK's internal shaded JLine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8535] Full std stream support for executor [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #2082:
URL: https://github.com/apache/maven/pull/2082
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8535] Full std stream support for executor [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2082:
URL: https://github.com/apache/maven/pull/2082#discussion_r1939322955
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -305,16 +307,29 @@ protected BuildEventListener doDetermineBuildEventListener(C context) {
}
protected void createTerminal(C context) {
+ boolean stdStreamsRedirected = System.console() == null
+ || context.invokerRequest.in().isPresent()
+ || context.invokerRequest.out().isPresent()
+ || context.invokerRequest.err().isPresent();
+ context.invokerRequest.err().ifPresent(err -> {
+ if (err instanceof PrintStream errPs) {
+ System.setErr(errPs);
+ } else {
+ System.setErr(new PrintStream(err, true));
+ }
+ });
if (context.terminal == null) {
MessageUtils.systemInstall(
builder -> {
- builder.streams(
- context.invokerRequest.in().orElse(null),
- context.invokerRequest.out().orElse(null));
- builder.systemOutput(TerminalBuilder.SystemOutput.ForcedSysOut);
- // The exec builder suffers from https://github.com/jline/jline3/issues/1098
- // We could re-enable it when fixed to provide support for non-standard architectures,
- // for which JLine does not provide any native library.
+ if (stdStreamsRedirected) {
+ builder.system(false);
Review Comment:
`system(false)` and `system(true)` are not really needed, this is implicit with the fact that there are streams set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8535] Full std stream support for executor [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2082:
URL: https://github.com/apache/maven/pull/2082#discussion_r1939321451
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -305,16 +307,29 @@ protected BuildEventListener doDetermineBuildEventListener(C context) {
}
protected void createTerminal(C context) {
+ boolean stdStreamsRedirected = System.console() == null
+ || context.invokerRequest.in().isPresent()
+ || context.invokerRequest.out().isPresent()
+ || context.invokerRequest.err().isPresent();
+ context.invokerRequest.err().ifPresent(err -> {
Review Comment:
Not sure why we should mess with `System.err` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8535] Full std stream support for executor [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2082:
URL: https://github.com/apache/maven/pull/2082#discussion_r1939451296
##########
impl/maven-executor/src/main/java/org/apache/maven/cling/executor/forked/ForkedMavenExecutor.java:
##########
@@ -87,12 +87,16 @@ protected void validate(ExecutorRequest executorRequest) throws ExecutorExceptio
@Nullable
protected Consumer<Process> wrapStdouterrConsumer(ExecutorRequest executorRequest) {
- if (executorRequest.stdoutConsumer().isEmpty()
+ if (executorRequest.stdinProvider().isEmpty()
+ && executorRequest.stdoutConsumer().isEmpty()
&& executorRequest.stderrConsumer().isEmpty()) {
return null;
} else {
return p -> {
try {
+ if (executorRequest.stdinProvider().isPresent()) {
+ executorRequest.stdinProvider().orElseThrow().transferTo(p.getOutputStream());
Review Comment:
Why do we consume stdin and write it to stdout ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8535] Full std stream support for executor [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2082:
URL: https://github.com/apache/maven/pull/2082#discussion_r1939643819
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -308,13 +309,13 @@ protected void createTerminal(C context) {
if (context.terminal == null) {
MessageUtils.systemInstall(
builder -> {
- builder.streams(
- context.invokerRequest.in().orElse(null),
- context.invokerRequest.out().orElse(null));
- builder.systemOutput(TerminalBuilder.SystemOutput.ForcedSysOut);
- // The exec builder suffers from https://github.com/jline/jline3/issues/1098
- // We could re-enable it when fixed to provide support for non-standard architectures,
- // for which JLine does not provide any native library.
+ if (context.invokerRequest.embedded()) {
+ builder.streams(
+ context.invokerRequest.stdIn().orElse(InputStream.nullInputStream()),
+ context.invokerRequest.stdOut().orElse(System.out));
Review Comment:
Why not `OutputStream.nullOuputStream()` ? If the user does not want output, let's not print it out.
Else, it will be difficult to work around. And user can simply pass in `System.out` if he wants.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org