commandline icon indicating copy to clipboard operation
commandline copied to clipboard

Built in Help and Version output goes to stderr not stdout.

Open oliverholliday opened this issue 5 years ago • 13 comments

The built in --help and --version outputs are printed to stderr instead of stdout, you have to redirect them to use the commands properly.

test --version returns the version number, but on stderr not stdout.

e.g. test --version 2>nul returns no output. test --version 2>&1 prints the version to stdout as expected.

oliverholliday avatar Jan 23 '19 17:01 oliverholliday

That's an excellent point.

ericnewton76 avatar Jan 23 '19 18:01 ericnewton76

Is there a drawback of directing the version and help screen to the standard error (stderr) No, why:

  1. The merit of writing to stderr is that the output will always be directed to the command line and it will always be written immediately to the display device. This means you will always see the output directed to stderr, as long as you have not reassigned stderrto another destination.

  2. stdout is fully buffered and stderr is not buffered. In other words, stdout will flush the buffer when the programmer explicitly asks for it or when it is most convenient, stderr writes the message immediately. ref: stdin, stdout, stderr

  3. No advantage for reassigning to stdout in windows, linux, Mac.

  4. Basically stderr and stdout are two different output streams, by default consoles display them both without redirection and both are manged by OS. By default, they both display on the terminal(but you can redirect them if needed).

  5. The Core developers classified the help and version as types of errors (By Design), and logically they can be displayed in the standard error stream (screen by default)

  6. In custom help, you can redirect to stdout (if needed)

Summary No loss if using stderr and No gain if using stdout. Both are directed to the screen. I think it's better to keep the output ASIS without change (By Design).

moh-hassan avatar May 08 '19 01:05 moh-hassan

@moh-hassan Would you say the 'fix' in #423 is unnecessary then, or does it still have merit as a way to help developers distinguish between explicit version/help errors and parser errors when implementing a custom help scenario?

The HelpWriter.Default could be modified to return new HelpWriter(Console.Error); so the default behavior of the library remains unchanged.

Then, if a developer wants to treat the results of explicit help/--help/version/--version calls as actual output that should go to Console.Out, he or she can do so through the ParserSettings:

settings = new HelpWriter(Console.Out, Console.Error)

twaalewijn avatar May 10 '19 10:05 twaalewijn

I dont think it's reasonable that printing out version or help output when specifically requested is counted as an error. If help output is triggered by invalid parameters then absolutely.

The GNU guidelines say the same.

Azure DevOps pipelines count any write to stderr as a pipeline error, because why wouldnt it - it's specifically signalling an error!

When I do an automated app installation I make it print the version output as a quick check for first run but using this library I have to redirect stderr to stdout, which means I can't possibly verify whether it worked or whether it legitimately errored.

Printing information to a screen is only one of the uses for standard streams, there are other use-cases for applications other than running them manually in a terminal. It makes sense to respect the standards and behave like other applications - i.e. only using stderr for printing errors.

oliverholliday avatar May 10 '19 11:05 oliverholliday

Sure and agree with you @twaalewijn and @oliverholliday that the wrong thing is to print version information to stderr, BUT, I concentrate on the side effect of the change and the gain we can get.

  1. This change can cause BREAKING CHANGE for all application using the CommandLine library. These application capture the normal output (real payload) in stdout and others (errors, version and help ) in stderr. So, we should think twice before doing this change.

  2. java starting from version 1 (1995) and up to now version 8 still continue send version to stderr Is there any special reason for the results of java -version going to stderr? A bug report here and their answer:

We should think very, very carefully before ever fixing this bug. It's obviously the wrong thing to print version information to stderr, but since we've been doing that since the beginning of time it seems likely that we'll break existing systems built on top of Java if we change it now.

and their resolution: "Wont Fix"

  1. Yes, The GNU coding standards state that commands should implement --version and that version info should be written to standard output. But POSIX standards don't mention this, nor do the LSB standards and is left to the application to select the streams.

From POSIX specifications for the standard streams:

At program start-up, three streams shall be predefined and need not be opened explicitly: standard input (for reading conventional input), standard output (for writing conventional output), and standard error (for writing diagnostic output).

In other words, errors, diagnostic information, and anything that falls into diagnostic category goes into stderr and version can be considered as a diagnostic output (not true payload output).

  1. In Pipeline operations: use the true payload of application in stdout others in stderr.

for example: when you pipeline the output of command1 to command2

     command1 --option | command2

you don't want the version message going to stdout, because it's now unexpected output that should not be processed by command2, so version can be in stderr.

  1. Why bothering if we can use stdout in custome help. Default parser give the minimum that we can get from the Parser and with custom parser we can generate our own help and version that can use stdout instead of stderr without Breaking Change.

moh-hassan avatar May 10 '19 15:05 moh-hassan

I don't believe that specifically requesting help output or a version string can be considered diagnostic output - in that case it is the payload. In all other cases I agree with you.

Given that it is a bug is it not worth fixing it and then documenting how to return the behaviour to how it is now should anybody require it.

oliverholliday avatar May 10 '19 16:05 oliverholliday

@twaalewijn, I didn't say the 'fix' in #423 is unnecessary, but it's nice if you can avoid the Breaking Change. Can you give working examples how to use the library when the PR is released with the cases you described here

moh-hassan avatar May 10 '19 22:05 moh-hassan

@moh-hassan Of course, I'd be happy to.

PR context

I'll start with some context and the assumption that the current behavior is considered a bug because that is how the PR is implemented right now.

In the PR I've replaced the HelpWriter property's TextWriter type with a new class called HelpWriter.

This HelpWriter class takes either one or two TextWriters to write help to. If only one TextWriter is passed all help gets written to that writer (which is similar how it currently works). If two TextWriters are passed, the results from 'Errors' that are caused by specifically entering help/version/--help/--version are written to the first writer and others to the second.

// All help written to Console.Error.
new HelpWriter(Console.Error);

// All help written to Console.Out.
new HelpWriter(Console.Out);

// help/version/--help/--version written to Console.Out and other errors to Console.Error.
new HelpWriter(Console.Out, Console.Error);

Next the ParserSettings of Parser.Default were changed to return a HelpWriter that behaves like the third example above:

// HelpWriter.Default currently lazily returns a new HelpWriter(Console.Out, Console.Error).
private static readonly Lazy<Parser> DefaultParser = new Lazy<Parser>(
    () => new Parser(new ParserSettings { HelpWriter = HelpWriter.Default }));

So the new standard would be that help/version/--help/--version results are written to Console.Out and other parsing errors still get written to Console.Error.

This is, as you rightfully pointed out, a breaking change for anyone using the default parser and expecting all Error class related output to go to Console.Error.

Current PR behavior

A developer that that wants to disable all help/error text can still continue using the old way to do so:

// Setting the HelpWriter to null still prevents any help from getting written
// so no change in this situation.
// This also means that developers who write their own completely custom help/error texts
// are probably unaffected since they would have had to disable auto help this way anyway.
Parser parser = new Parser(settings => settings.HelpWriter = null);

A developer that was intercepting auto generated help will probably have assigned the HelpWriter on ParserSettings to their own TextWriter instance.

StringWriter myStringWriter = new StringWriter();

// Developers assign their own TextWriter to manage auto written help themselves.
Parser parser = new Parser(settings => settings.HelpWriter = myStringWriter);

These developers will find out that their code no longer compiles after updating the package. This was done intentionally so they are notified of the changed behavior of being able to differentiate between help/version/--help/--version output and other parsing errors.

If they want to keep managing their help this way without any changes they can simply instantiate a HelpWriter with their current TextWriter.

StringWriter myStringWriter = new StringWriter();

// Developer still wants all help to be written to their own own TextWriter.
Parser parser = new Parser(settings => settings.HelpWriter = new HelpWriter(myStringWriter));

If they want to keep managing the help but differentiate between help/version output they can filter both streams by adding another TextWriter.

StringWriter helpVersionStringWriter = new StringWriter();
StringWriter errorStringWriter = new StringWriter();

// Developer wants to do something with the auto generated help
// but filter between help/version verb/option output and parsing error output.
Parser parser = new Parser(
    settings => settings.HelpWriter = new HelpWriter(helpVersionStringWriter, errorStringWriter));

Breaking change workaround

Now, if it were to be decided that this issue is in fact not a bug but something that was done by design and changing it would cause a breaking change that cannot be accepted without bumping the major version number to a 3, I could change the PR with what I suggested in my earlier comment.

This would stop the breaking change from occurring because the HelpWriter.Default that is used by the default Parser would still write everything to Console.Error:

// HelpWriter.Default would lazily return new HelpWriter(Console.Error).
// No effective behavior changes for developers that only use the default parser.
private static readonly Lazy<Parser> DefaultParser = new Lazy<Parser>(
    () => new Parser(new ParserSettings { HelpWriter = HelpWriter.Default }));

As a result, myself and others like @oliverholliday who want to count output that was triggered by calling the help/version verbs or --help/--version options specifically as output of a command (and thus something that needs to be written to Console.Out), would still be able to easily do so by assigning a HelpWriter that does this to the ParserSettings:

Parser parser = new Parser(
    settings => settings.HelpWriter = new HelpWriter(Console.Out, Console.Error));

I'd still prefer the behavior of how the PR is implemented now because personally I consider it a bug that specific calls to the help/version verbs and options were ever written to Console.Error.

Having said that I also agree that potentially breaking scripts that were counting on everything being written to Console.Error without proper warning could be undesirable. Which is arguably the case when using the default parser if a developer does not read the eventual changelog.

So I could live with setting settings.HelpWriter = new HelpWriter(Console.Out, Console.Error) for now as long as an eventual v3 of the library changes it to be the default again.

I think this covers all the use cases but if it did not and you have more questions please just ask.

twaalewijn avatar May 11 '19 14:05 twaalewijn

@twaalewijn +10 Excellent explanation. Your comment answered many questions.

I think starting with the default (directing version/help to stderror) is wisdom to start without breaking change. This enable the library continue working without Breaking Change. Developers can use the new feature in the custom help as you described above (wiki can be updated).

Just a suggestion:

  • You can add the Obsolete attribute [Obsolete] to the methods /properties that is can be removed in the future. So, this is an early warning of the change and developers will be ready for the change.

           [Obsolete("This method/property will be deprecated in the next release. Use Method/property instead.")]
          private HelpWriter helpWriter;
    
  • You can refactor the code and control the different options using enum like:

            //The enum control the creation of HelpWriter
      enum HelpStreamEnum
      {
        none,  // ==>settings.HelpWriter = null		 
        StdError,  // All help written to Console.Error.==> {new HelpWriter(Console.Error);}
        StdOutput,  // All help written to Console.Out.==> { new HelpWriter(Console.Out);}
        Mixed      // help/version/--help/--version written to Console.Out and other errors to Console.Error. ==>{new HelpWriter(Console.Out, Console.Error);}
      }
    
  • You can keep the current property 'HelpWriter helpWriter' in ParserSettings class ,temporary and can be removed in the future, and marked as [obsolete "use the HelpStreamEnum instead"] , so developers can continue use the library ASIS without breaking change.

  • Add a new property to the ParserSettings class:

        HelpStreamEnum  HelpStream {get;set;} 
    

The ParserSettings can be like:

	public class ParserSettings : IDisposable
	{
		//.... other properties as is
		[obsolete "The HelpWriter property will be deprecated in the next release.use the HelpStreamEnum instead"] 
		private HelpWriter helpWriter;
		private HelpStreamEnum helpStream; //new to control HelpWriter creation
		//.... other properties as is
	}

So, developers can use the new feature like

         // Developers assign their own HelpStream to manage auto written help themselves.
         //the new way
	Parser parser = new Parser(settings => settings.HelpStreamEnum = mixed);

You control the output stream based on the HelpStreamEnum value and create HelpWriter as commented above.

In this way the transition to the new feature is smooth and the new feature go side by side to the current behavior.

moh-hassan avatar May 12 '19 02:05 moh-hassan

@moh-hassan I'm all for making some more default options available to developers through a new enumeration.

Before I implement it in the PR though could you clarify question I have about your example?

If the HelpWriter property is marked as Obsolete, what would be the migration path for developers currently intercepting the written help text with their own TextWriter?

A consumer of this pattern is the unit test project because it tests the written auto help this way. Much in the same way as not breaking things for developers using the default parser and user scripts expecting all help/error output to go to stderr, not providing a way to intercept help text using the enum property is a breaking change in my opinion (albeit on the developer side of things).

twaalewijn avatar May 17 '19 11:05 twaalewijn

If the HelpWriter property is marked as Obsolete, what would be the migration path for developers currently intercepting the written help text with their own TextWriter?

For the developers who are using custom Parser and configure ParserSettings like:

     var parser= new Parser(config=>config.HelpWriter=null); //implement their own help
   

They will get a compilation Warning CS0672

Program.cs(34,17,34,27): warning CS0618: 'ParserSettings.HelpWriter' is obsolete: 'The HelpWriter property will be deprecated in the next release.use the HelpStreamEnum instead'

So, they may start modifying their code to use the new HelpWriter as described in wiki

The Migration path:

The PR when merged,I expect, the old and new helpwriter are available so no breaking change occur in their code or in current unit test.

The entry of auto help is this method:

         private static ParserResult<T> DisplayHelp<T>(ParserResult<T> parserResult, TextWriter helpWriter, int maxDisplayWidth)

Which you modified to other (overloading) method:

        private static ParserResult<T> DisplayHelp<T>(ParserResult<T> parserResult, HelpWriter helpWriter, int maxDisplayWidth)

These two methods can go side by side and help can be generated using the old or new helpwriter class and current /new unit test pass.

In a major release it will be only the new helpwriter. The PR can be availabele in the next minor releae without breaking change for auto or custome help I think a wiki page can be available from now to describe the procedure to migrate/use the new help with examples.

If you have a different opinion we can discuss it and agree together to the best migration approach.

Also all developers are welcome to share their opinion in the quickest and safe migration path to use the new helpwriter.

moh-hassan avatar May 18 '19 02:05 moh-hassan

Azure DevOps pipelines count any write to stderr as a pipeline error, because why wouldnt it - it's specifically signalling an error!

This is a bug in the Azure DevOps pipeline, because stderr is often used for other things besides errors. Many programs use stderr to print progress messages, with stdout being reserved for the output that might be passed down a pipe. So Azure DevOps is doing the wrong thing here. Though CommandLineParser's current behavior (printing help to stderr by default) is also wrong and needs to default to stdout in version 3.

rmunn avatar Jun 04 '20 03:06 rmunn

Are there going to be any changes related to this in 2.x?

naavis avatar Nov 01 '21 12:11 naavis