fleipold / jproc

Java library that helps with running external processes.
BSD 3-Clause "New" or "Revised" License
194 stars 23 forks source link

I would like jproc to have the option to ignore non-zero exit codes #2

Closed mgg4 closed 8 years ago

mgg4 commented 8 years ago

I have just started using jproc, and for most things I'm finding it easy to use, and trouble-free. However there are some commands that I run that are expected to return a non-zero exit, even when they work correctly. What I'm thinking about is a call to Proc that includes a flag as to whether the exit code should be checked. If this flag is false, then don't even look at the exit code in line 77 of Proc.java.

I would be happy to fork the code and try to deliver a pull request to you if you agree.

fleipold commented 8 years ago

This sounds like a very useful addition to me. I would like the current behaviour as the default, but the builder could surely get a new method, e.g. ProcBuilder#withDoNotThrowOnNonZeroExitCode(). I am happy to accept a pull request.

mgg4 commented 8 years ago

I was thinking about this a bit more, and I think I came up with something that might be even more useful. The idea came to me when I found that Chef provides a "returns" parameter for the commands it runs. This parameter lists the expected exit codes from the command. It made me think this could be more useful than just a flag to completely suppress the throw of the exception.

To make this work, changes would be required to the ProcBuilder class as well as the Proc Class

The current Proc() constructor currently has 7 parameters:

public Proc(String command,
            List<String> args,
            Map<String, String> env,
            InputStream stdin,
            OutputStream stdout,
            File directory,
            long timeout)

This constructor would be left fairly well intact. A new Proc constructor would be created to add an 8th parameter:

public Proc(String command,
            List<String> args,
            Map<String, String> env,
            InputStream stdin,
            OutputStream stdout,
            File directory,
            long timeout,
            int[] returns)

The original Proc() constructor would be essentially a wrapper around this new constructor, supplying [0] for the new parameter. This will permit the backwards compatibility for those programs that call Proc() directly. The logic change for the new constructor would replace the current check for exit code of zero with a check to see if the exit code is in the array provided. If not, throw the exception. A useful way of turning off the the check entirely would be to pass an empty array. This could be used by the program to disable the throw of the exception entirely.

In addition, on the ProcBuilder class, a new attribute would be created:

int[] exitcodes = [0];

Further, a new helper function would be created to override the default:

public ProcBuilder withExpectedExitCodes(int[] exitcodes)

The ProcBuilder.run() methods would be updated to use the new 8-parameter call to Proc(), and would include either the default list of return codes (which provide the current behavior), or the override list of valid exit codes.

To summarize, the use of the array gives us the ability to Turn off the throw of the Exception entirely (empty array) List the acceptable exit codes for the command (non-empty array) Maintain the current behavior as the default (array initialized to [0])

Please let me know if this looks like a better solution. If so, I'll get started on the changes later today. I'm currently working on a hack-week project here at Citrix, and I ran into this issue. Making and testing the changes locally will greatly simplify my project, and will provide a test-bed for the changes prior to submitting the pull request to you.

fleipold commented 8 years ago

Thanks for your input!

I like the solution. One thing that is a bit weird are the semantics of the empty array as allowing anything. It's a pragmatic solution, but we could make it explicit by adding the method I suggested to set the array to an empty array. I saw your commit and will add some comments there.

mgg4 commented 8 years ago

That would work. Perhaps something like: .ignoreExitCodes()

Does that work for you? I can add it to my next commit before I issue the pull request.

fleipold commented 8 years ago

Sure. Sounds great. On 21 Apr 2016 22:17, "mgg4" notifications@github.com wrote:

That would work. Perhaps something like: .ignoreExitCodes()

Does that work for you? I can add it to my next commit before I issue the pull request.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/fleipold/jproc/issues/2#issuecomment-213211583

mgg4 commented 8 years ago

I was finally able to test my first round of local changes. Successfully. I will be completing my hackweek project and then return to the next round of changes on jproc. It will probably be next week before I can give you the pull request for all the changes.

mgg4 commented 8 years ago

Is there a process for the new version to be made available in the Mvn Repository? What version will be used for that?

fleipold commented 8 years ago

I released a new version. Also, did some massaging of the interface. It should be available in a couple of hours as 2.1.0.

mgg4 commented 8 years ago

I see your changes and concur with your cleanup. I am still learning the ins and outs of Java coding, and I thank you for teaching me a bit more about writing tight Java routines. The Helper template is not a concept that I use or see daily, so it didn't occur to me to use that. In addition, I like the rework of the way status codes are passed in; although I will need to update my code, it will actually make the next step a bit easier, so thank you.

It was my pleasure to work with you.