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

[JENKINS-52729] Launcher.ProcStarter.stdout(TaskListener) discards remotability #3563

Merged

Conversation

2 participants
@jglick
Copy link
Member

commented Jul 24, 2018

See JENKINS-52729.

I suspect there is also a lower-priority encoding bug here, but I am not bothering with that for now.

Proposed changelog entries

  • Developer: Launcher.ProcStarter.stdout(TaskListener) did not properly send its argument over a Remoting channel to an agent.

@jglick jglick requested a review from oleg-nenashev Jul 24, 2018

jglick added some commits Jul 24, 2018

@jglick jglick changed the title [JENKINS-52729] Demonstrating issue with Launcher.ProcStarter.stdout(TaskListener) [JENKINS-52729] Launcher.ProcStarter.stdout(TaskListener) discards remotability Jul 24, 2018

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

Sigh, apparently echo is not an actual command on Windows, but built into cmd.exe? Presumably so you can @echo off. Whereas on Linux it is both. Will fix test accordingly.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

FTR this call seems to be what would be used by typical freestyle build steps.

@jglick jglick referenced this pull request Jul 25, 2018

Closed

[JENKINS-52692,JENKINS-38313] - External task logging API #3557

0 of 4 tasks complete
@oleg-nenashev
Copy link
Member

left a comment

Yes, it would be nice to get it fixed in the context of External Logging stories. In #3557 reference impls I work it around by creating my own Launcher, but such approach would be much better

@@ -295,7 +297,9 @@ public ProcStarter stdout(@CheckForNull OutputStream out) {
* @return {@code this}

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 31, 2018

Member

It makes sense to explicitly set the requirement that the class is remotable in Javadoc

This comment has been minimized.

Copy link
@jglick

jglick Jul 31, 2018

Author Member

All TaskListener implementations are expected to be remotable but I will clarify this in Javadoc.

@oleg-nenashev
Copy link
Member

left a comment

By the way, stederr() has identical problem. I suggest fixing it here as well so that we do not get into surprises with some implementations really using 2 streams

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

OTOH we could create a follow-up task for stederr

jglick added some commits Jul 31, 2018

@jglick jglick removed the needs-fix label Jul 31, 2018

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

stderr() has identical problem. I suggest fixing it here as well

It does not—there is no stderr(TaskListener) overload. If we wished to allow a distinct and remotable stderr stream, that would be a new API which plugins would have to call.

surprises with some implementations really using 2 streams

The implementations we care about (CommandInterpreter, SCM plugins, etc.) do not separate stderr. I cannot find any callers using this method from a brief search.

There are some calls which currently call stdout(OutputStream) which would not benefit from this change, such as Maven.perform, but that is out of scope.

I did find one possible bug in the mercurial plugin: if you call stdout(TaskListener) and then stdout(OutputStream) you expect the latter call to cancel the former.

A call to stdout(TaskListener) should be countermanded by a call to s…
…tdout(OutputStream), which was true for local but not remote launchers.

@jglick jglick requested a review from oleg-nenashev Jul 31, 2018

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

Test failure is a common flake:

java.lang.AssertionError: deletion stopped
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.junit.Assert.assertFalse(Assert.java:64)
	at hudson.UtilTest.testDeleteRecursive_onWindows(UtilTest.java:489)
@oleg-nenashev
Copy link
Member

left a comment

OK, let's merge it as is. Public API does not change here, so we can fix STDERR in a follow-up if I discover the usa-case for it

@oleg-nenashev oleg-nenashev merged commit ebd7111 into jenkinsci:master Aug 2, 2018

1 check failed

continuous-integration/jenkins/pr-merge This commit has test failures
Details

@jglick jglick deleted the jglick:RemoteLaunchCallable-JENKINS-52729 branch Aug 6, 2018

@jglick jglick referenced this pull request Aug 6, 2018

Merged

Updated JEP-210 #166

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.