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

Change command error stream prefix to STDERR. #688

Merged
merged 1 commit into from Dec 1, 2014

Conversation

Projects
None yet
3 participants
@mmb
Copy link
Contributor

commented Oct 30, 2014

Currently output written to stderr by a process shows up in the console
log with the prefix "ERROR: ". This can cause the user to interpret all
messages written to stderr as errors and many are just informational.
This change will help users make this distinction.

This is a personal pull request and not associated with Pivotal.

@srinivasupadhya srinivasupadhya modified the milestone: Release 14.3.1 Nov 3, 2014

@arvindsv

This comment has been minimized.

Copy link
Member

commented Nov 22, 2014

Hi @mmb, sorry for the delay. I was traveling, and I'm slowly getting back to emails and stuff.

I think this test needs to be changed. Also, the test that you wrote (shouldPrefixStderrOutput) failed for me:

com.thoughtworks.go.util.command.CommandLineException: Error while executing [cd /a/directory/that/does/not/exist] 
 Make sure this command can execute manually.
    at com.thoughtworks.go.util.ProcessManager.startProcess(ProcessManager.java:106)
    at com.thoughtworks.go.util.ProcessManager.createProcess(ProcessManager.java:62)
    at com.thoughtworks.go.util.command.CommandLine.createProcess(CommandLine.java:367)
    at com.thoughtworks.go.util.command.CommandLine.execute(CommandLine.java:360)
    at com.thoughtworks.go.util.command.CommandLineTest.shouldPrefixStderrOutput(CommandLineTest.java:413)
    at com.googlecode.junit.ext.JunitExtRunner.invokeTestMethod(JunitExtRunner.java:41)
Caused by: java.io.IOException: Cannot run program "cd": error=2, No such file or directory
    at java.lang.ProcessBuilder.start(ProcessBuilder.java:1041)
    at com.thoughtworks.go.util.ProcessManager.startProcess(ProcessManager.java:99)
Caused by: java.io.IOException: error=2, No such file or directory
    at java.lang.UNIXProcess.forkAndExec(Native Method)
    at java.lang.UNIXProcess.<init>(UNIXProcess.java:135)
    at java.lang.ProcessImpl.start(ProcessImpl.java:130)
    at java.lang.ProcessBuilder.start(ProcessBuilder.java:1022)

I'll take a look at it again soon, but if you know why it failed or can use something like echo instead of cd, maybe it'll work? Or, maybe it's something weird with my box.

@mmb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2014

Thanks for taking a look @arvindsv. I will fix that test.

For the test I wrote I was trying to find a command that would exist everywhere everywhere the test runs but would fail because of incorrect arguments. I'll try echo. These tests must work on Linux, Windows and OSX correct?

@arvindsv

This comment has been minimized.

Copy link
Member

commented Nov 25, 2014

They do need to work on Linux and Windows, for sure. That's because the build on build.go.cd runs on those two. However, a lot of people use OSX for development, so it should ideally work on that as well. If you're finding it hard to get something which runs on all OSes, then there is some test infrastructure which can help you (since this problem has been faced before).

Take a look at these two tests: this and this. The first one is set to run on every OS other than Windows and the second is set to run only on Windows. So, that's an option too. You would have needed to add this line at the beginning of the test, but in CommandLineTest where you added that new test, that line already exists.

Change command error stream prefix to STDERR.
Currently output written to stderr by a process shows up in the console
log with the prefix "ERROR: ". This can cause the user to interpret all
messages written to stderr as errors and many are just informational.
This change will help users make this distinction.

@mmb mmb force-pushed the mmb:stderr-output-prefix branch from dfc1b87 to 2d86747 Nov 30, 2014

@mmb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2014

I fixed the perforce test and changed the command from cd to rmdir.

Hopefully rmdir should exist as a non built-in command on all platforms and write something to stderr when it is called with a directory that does not exist.

arvindsv added a commit that referenced this pull request Dec 1, 2014

Merge pull request #688 from mmb/stderr-output-prefix
Change command error stream prefix to STDERR.

@arvindsv arvindsv merged commit fec0ce3 into gocd:master Dec 1, 2014

@arvindsv

This comment has been minimized.

Copy link
Member

commented Dec 1, 2014

Makes sense. Let's see. :) I've merged it.

@arvindsv arvindsv added this to the Release 14.4 milestone Dec 2, 2014

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.