NuGetPush alias should capture NuGet.exe output

[hienng]

When pushing with NuGetPush and expecting something like a failure because of a disabled package override on the NuGet Feed.

The expected NuGet.exe output is: Response status code does not indicate success: 403 (Package Already Exists)

However it is not captured and instead the actual return in OnError(exception => { ... }); is: NuGet: Process returned an error (exit code 1)

Relevant Gitters: November 8, 2018 8:10 AM November 8, 2018 9:53 AM November 8, 2018 11:20 AM

[fseegraeber]

After looking at different tool aliases, this seems to be the current behavior of most (all?) tools. https://github.com/cake-build/cake/blob/c39e42433275f6cbc39d565e6a3bc341fc07aec4/src/Cake.Core/Tooling/Tool.cs#L133-L141 If the exit code is not zero you'll get this generic error exception.

To get a hold of the standard output or error you'd need to run NuGet from a ProcessRunner and set ProcessSettings.RedirectStandardOutput/RedirectStandardError afaik. But that defeats the whole purpose of aliases, I guess.

It would probably be nice to get the standard error output in the exception message of a tool process that exits with != 0. I'm not too sure about a clean way to do that though. We could default to redirecting the the standard error and pass that to the ProcessExitCode method.

[devlead]

There's a few issues with always redirecting output, you would lose console log unless you proxy it, if you proxy it you would lose colors in output. Some aliases already redirect and uses output so you would need to take that into consideration too.

One could potentially do a proxy IProcess as a module to test that outside Cake.Core.

Perhaps it could be optin via toolsettings and perhaps it's enough to proxy stderror and use that as message if avail.

[fseegraeber]

It would have to be a proxy ProcessRunner, as that is the one actually handling the redirect. https://github.com/cake-build/cake/blob/c39e42433275f6cbc39d565e6a3bc341fc07aec4/src/Cake.Core/IO/ProcessRunner.cs#L158-L170 Combining that with a opt-in via ToolSettings means that each alias would need to check the settings and pass the proper ProcessRunner to the constructor.

Maybe its arguable to drop the proxying under the assumption, that one either wants the console output or a proper message in OnError(). E.g. extending ToolSettings with a RedirectStandartError property and have Cake.Core.Tooling.Tool set that in the ProcessSettings and use the stderror as a part of the message.

To me that still doesn't seem ideal though. Especially since stderror in ProcessWrapper currently is a queue. Consuming that to get the message would prevent aliases from using it in a post action.

[fseegraeber]

Looking around this seems to be a possible duplicate of #1423. It's also somewhat related to #2270 .

If there was a way for Tool to pass a handler for stderror it would be easy to grab an error message and possibly print the output itself.

[tapika]

Please note that class NuGetPusher is sealed, so end-user cannot even override it. Also not so clear on where anykind of error handling should be located (OnError does not contains sufficient information), suspect it's located inside ProcessRunner > ProcessWrapper, which is intermediate class contructed on the way.

It would be nice to report - if you have authentication failure, please contact this and that guy or configure nuget like written in wiki pages.