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

When process returns non-zero exit code, it's not possible to get output. #103

Closed
Erwinvandervalk opened this issue Jan 4, 2019 · 13 comments
Labels
question Further information is requested

Comments

@Erwinvandervalk
Copy link

When executing a dotnet nuget publish, I get a 409 status if the package already exists. I'm trying to get the output, but this is only returned when the process returns a 0 status code. It could be a handy property on the non zero statuscode exception.

Something like:

        public static async Task<string> ReadAsync(string name, string args = null, string workingDirectory = null, bool noEcho = false)
        {
            using (var process = new Process())
            {
                process.StartInfo = ProcessStartInfo.Create(name, args, workingDirectory, true);
                await process.RunAsync(noEcho).ConfigureAwait(false);

                var output = await process.StandardOutput.ReadToEndAsync().ConfigureAwait(false);

                if (process.ExitCode != 0)
                {
                    process.Throw(output);
                }

                return output;
            }
}

If you want, I can create a PR that adds this.

@adamralph adamralph added the question Further information is requested label Jan 5, 2019
@adamralph adamralph self-assigned this Jan 5, 2019
@adamralph
Copy link
Owner

adamralph commented Jan 5, 2019

@Erwinvandervalk thanks for raising this.

There are a few things to consider here:

  • The 409 status code will probably be written to stderr, not stdout. Or at least it should be, if dotnet publish is well behaved.
  • It doesn't make sense to call Read for dotnet nuget publish, since you don't actually want to read the output. What would you do with the return value? It makes sense to call Run for dotnet nuget publish.
  • We could introduce optional TextWriter parameters for stdout and stderr, but in each case, the text would be suppressed from the console, and that's probably not desirable. Bear in mind that stderr is misleadingly named. It's not just for the final error that occurs, it's for all diagnostic information that is shown while the command is running. Essentially, anything that is not part of the output of the command. I'm not sure that dotnet publish logically has an "output", so I would assume that everything it writes the console is actually going to stderr.

Another thing to consider is the exit code of dotnet nuget publish. Does that convey anything meaningful, or is it always the same number regardless of which problem occurred? If the former, you could catch NonZeroExitCodeException, which has an ExitCode property.

@adamralph
Copy link
Owner

BTW - if the process exit code is currently not meaningful, it could be worth raising an issue with NuGet to change that. It would be much simpler to inspect the exit code than to tinker with stdout/stderr, and it would require no changes in SimpleExec.

@Erwinvandervalk
Copy link
Author

Erwinvandervalk commented Jan 5, 2019

Totally agree with your comment that the exit code should convey what kind of problem there is.

I will dig into dotnet nuget a bit more to see if they return a specific error code, but I doubt it.

But from the perspective of SimpleExec library, I do think it's valuable to have access to the stdout and stderr. Parsing output is a often horrible solution (some machines might have different language settings for example), but sometimes you don't have much of a choice to get a use case to work. Will follow up with a suggestion :)

@adamralph
Copy link
Owner

I guess it won't hurt to set the stdout received so far as a property on the exception. However, I'm not sure that will help you. If dotnet nuget publish is behaving correctly, the error will be written to stderr and, for the reasons stated above, I'm not sure it's wise to capture stderr.

@adamralph
Copy link
Owner

@Erwinvandervalk did you get anywhere regarding getting a specific exit code from dotnet nuget? As I said, I believe that's the best solution.

@Erwinvandervalk
Copy link
Author

It doesn't look like nuget.exe does that unfortunately. I dived into the source code of nuget.exe. It doesn't appear to use any exit code other than 1 for failures.

https://github.com/NuGet/NuGet.Client/search?q=ExitCodeException&unscoped_q=ExitCodeException

@adamralph
Copy link
Owner

@Erwinvandervalk I suggest that you propose this change to the NuGet team. If you raise an issue, I'd be more than happy to chime in with some supporting comments.

@Erwinvandervalk
Copy link
Author

Created an issue here: NuGet/Home#7723

@adamralph
Copy link
Owner

👍 @Erwinvandervalk you've captured the problem nicely in that issue. FWIW, I think NuGet/Home#1630 is also a good, perhaps better, solution.

@adamralph adamralph removed their assignment Feb 4, 2019
@adamralph
Copy link
Owner

Hey @Erwinvandervalk, seeing as the best fix we've identified lies in NuGet, I'm closing this. Thanks again for raising the question.

@adamralph
Copy link
Owner

adamralph commented Jun 2, 2019

@Erwinvandervalk good and bad news!

The good news is that NuGet/Home#1630 was released in NuGet 5.1.

The bad news is that, while NuGet 5.1 is stated as being included in .NET SDK 2.1.70x and 2.2.30x, dotnet nuget push doesn't yet support the new feature. 🙄 🤦‍♂

@adamralph
Copy link
Owner

FTR NuGet/Home#1630 was propagated to the .NET SDK in at least version 3.1.200.

image

@adamralph
Copy link
Owner

@Erwinvandervalk FYI in the new API in version 9 both standard output and standard error are available on ExitCodeException, and are included in its message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants