[Invalid] CLI returns with 0 exit code under err conditions

[fonkprop]

This is a bug that's been discussed a number of times before:

http://forum.handbrake.fr/viewtopic.php?f=24&t=6883

http://forum.handbrake.fr/viewtopic.php?f=6&t=15422

In both cases, jbrjake has simply declared the bug reports invalid. Let me be totally clear about this: jbrjake has no idea what he's talking about, and it's a shame that HandBrakeCLI is broken because of his obstinacy and ignorance. No correct command line tool returns with a zero exit status when interrupted midway through its task with a ctrl-c, for example. Try it with any Unix tool. This error has caused HandBrakeCLI to be much harder to script than it should be.

How should this be fixed? Simply put, a zero exit status should ONLY be returned when transcoding has completed successfully. In ALL other cases, a non-zero exit code should be returned.

Please fix this, and make it easier for an ecosystem of scripting tools to grow up around HandBrake.

[TedJ]

I'll be the first to say this. Since you appear to be knowledgable about these things, we eagerly await your patch.

The longer answer is that the Handbrake CLI was never really intended for widespread use in production environments, it's primary function is to act as a testbed for new filters, etc. without needing to add extra functionality to one of the GUI's. It's kept in a folder called test/ for a reason.

[fonkprop]

Hey, I'm happy with "Look, we recognize it's a problem, but we don't have time to fix it - please submit a patch.". That's totally fine, and if that was the starting point, this report would have come with a patch. What's not fine is for one of the developers to ignorantly (and rather rudely) claim that this is, in fact, the correct behaviour. If you do a search on a site like github for HandBrakeCLI, you'll quickly see that there are a large number of projects using this tool, and all of them have either had to resort to hideous kludges to deal with this problem, or are buggy because they haven't bothered, so jbrbrake's bad attitude has has wasted a lot of people's time.

Also, the project devs should embrace the fact that the CLI is a widely used interface to the project, and move it out of the "test" directory, whatever feeble symbolic value that has. Most people will download the CLI from the website in executable form, so the fact that the tool is in the "test" directory in the source means jack to them.

[Deleted User 11865]

Hey, I'm happy with "Look, we recognize it's a problem, but we don't have time to fix it - please submit a patch.". That's totally fine, and if that was the starting point, this report would have come with a patch. What's not fine is for one of the developers to ignorantly (and rather rudely) claim that this is, in fact, the correct behaviour.

It is the correct behavior because the devs decided it was. If you don't like it, patch/fork the CLI for your own usage.

Also, the project devs should embrace the fact that the CLI is a widely used interface to the project, and move it out of the "test" directory, whatever feeble symbolic value that has. Most people will download the CLI from the website in executable form, so the fact that the tool is in the "test" directory in the source means jack to them.

The fact that the CLI is widely used may be true, so the devs could decide to make the CLI more friendly to some users. Last time I checked though, they are still free to do whatever they like, so if they decide not to (because they don't care or for any other reason), well, there's pretty much nothing you can do about it.

[TedJ]

It's not simply a question of not having time to fix it, there may also no interest in fixing it... and there is nothing wrong with this. The very nature of any open source project is that it is a collaborative effort where several developers pursue their own interests.

Let me present an example. A recent contributor to the project is davidfstr who wanted to improve subtitle support, specifically adding extended support for SSA subs. So he contacted the developers, submitted some patches, showed the commitment to maintain these changes and as a result his code has been in SVN for several months.

So stating that there are many projects out there that use HandbrakeCLI and have to resort to hideous kludges doesn't garner any real sympathy. Any one of these downstream projects could have contributed code back to the project to fix the issue, but have failed to do so. I believe that there was some work being done to add libhb error handlers, but the developer in question hasn't had much time to tend to it since moving from New Zealand to Australia with his family.

I will admit that jbrjake can come across as acerbic, but when people talk to him like an indentured servant rather than someone who contributes his time and efforts to this project on a voluntary basis I can't say I blame him.

[fonkprop]

Look, I don't have a problem with acerbic. What I have a problem with is someone who is ignorant but won't listen. Re-read those two links above - why would anyone from outside the project bother to do the work to create a patch to fix this behaviour if one of the devs on this project has taken a stance this ill-informed and wrong-headed? It's pretty clear that if it's up to this guy, the time would be wasted. What I'm hoping is that one of the more knowledgeable devs on this project will look at this, realise that the behaviour is broken, and say so on this bug report. I'll bet you $5 that a patch will follow pretty quickly as soon as it's on record that there's impetus to fix this - either from myself, or from someone else.

[jbrjake]

How should this be fixed? Simply put, a zero exit status should ONLY be returned when transcoding has completed successfully. In ALL other cases, a non-zero exit code should be returned.

...and this is exactly what HandBrakeCLI is doing. If transcoding completes successfully, it returns a zero exit status. If there is some failure that prevents it from completing to write a transcoded file, it errors. You are conflating user-shortened files with actual processing errors. If, when you hit ctrl-c, HB just stopped what it was doing and exited, leaving an unreadable file, what you are saying would make sense. But as I explained, in detail, in the previous threads....it simply is not the case. Ctrl-c does not break the process, it ends it gracefully by putting in motion a set of calls to finish muxing the file and leave completed output that can be played as video. This is not what most Unix tools do on ctrl-c, so perhaps that is why you assume all should respond exactly the same way.

Your personal comments towards me were totally unnecessary.

[JohnAStebbins]

FYI, I can see where the request for an error on ctrl-c could be coming from. This is something a person writing a batching script would want. A batching script running in a loop over a large collection of files is going to need some way to detect if the user tried to terminate by hitting ctrl-c. Without a check for some error code from the program, the script is just going to continue on to the next file. User has to hit ctrl-c for every file till the script reaches the end of the list.

[jamiemlaw]

I assume that:

1. For portability reasons, HandBrake can have only the two exit codes: 0 and 1
2. The codes are used more in debugging HandBrake than in other users' scripts

If both of those are true, I say the devs are well within their rights to keep the codes as they are (despite it being rather annoying for users in these certain cases); else, is there a way to return a third status to signal that HandBrake functioned correctly, but suspects that the user did not?

[jbrjake]

As I mentioned in the prior threads, it seems like the best solution for this would be to utilize the error and warning handling* eddyg added to libhb. That way actual information can be communicated to the user instead of an inscrutable error code.

* Pretty sure hb_warn() never made it in but there's no reason it couldn't. Then if ctrl_c is called we could have it tell libhb that the die=1 came from cancelling a job, and in work.c's cleanup it would just be: hb_warn( "User has ended encoding early; dying gracefully. Muxing has been completed and memory freed.");

Not hard to scan stderr for lines starting with "WARNING:". Could even have all the strings in a nice little enum that the scripters can stay in sync with. But then, it's not hard to scan for lines starting with "Encoding Quit by user command." which it already does on early ending....

Sure, batch encoders need a way to know when to stop, but IMO abusing the exit status to indicate an error has occurred when one has not, just because it's easy to monitor, isn't the way to go.

[fonkprop]

jbrjake, it's clear that you are so committed to being wrong on this point, that no amount of argument and no flood of examples of how this should work will convince you. This is as I expected, based on your past performance, and your arrogant treatment of people who have tried to reason with you about this before. The comments below are for other people reading this bug.

Nearly all Unix tools catch SIGINT (which is raised when you press ctrl-c) to do some sort of graceful shutdown. None the less, nearly all Unix tools then return an exit status. For example, the very simplest tool that acts on a stream in the way that HandBrakeCLI conceptually does, is the unix dd tool. Here's the source of the GNU version of dd:

http://git.savannah.gnu.org/cgit/coreut ... e/src/dd.c

Notice that it handles SIGINT, shuts down gracefully, and then returns an error code by re-raising the signal. Like HandBrakeCLI, it also leaves a partially completed output file, if one was specified. The situation is exactly analogous, but dd does it right, and HandBrakeCLI does it wrong.

The decision to stick to the broken behaviour the tool current has means that there is NO defined and futureproof way to detect if a transcoding was interrupted at the terminal from a script. No, not even grepping output for "100%", like some tools do, because this relies on a minor implementation detail that could change between versions of HandBrakeCLI. In my case, it meant that it was easier for me to just use other tools to transcode my video collection for my AppleTV, which is a shame.

[s55]

fonkprop - pot... kettle... black.

How should this be fixed? Simply put, a zero exit status should ONLY be returned when transcoding has completed successfully.

That's a matter of opinion.

[fonkprop]

No, it's not a "matter of opinion". The way you currently do this is simply unarguably wrong. I really don't understand the resistance to this - the problem has been reported multiple times, it's an easy and obvious fix, and doing the right thing will benefit the project. Clearly ignorance has become a matter of pride in this case.

[s55]

This has nothing to do with pride. You don't understand the context of this tool.

Don't mis-understand me here. I don't disagree with you (within context)
It would certainly be useful however I would much rather see a user-orientated CLI developed with a whole number of issues fixed so that HandBrakeCLI can be left in it's "brutal by design" state.

[fonkprop]

Wouldn't it be easier to just incrementally grow the tool you already have? If the CLI tool is intentionally useless because it's just for testing, why do you offer compiled binaries for download from the website without warning users?

Do a search on github or using Google codesearch, and you'll see that the CLI is already widely used, and that nearly all the scripts using it are kludgy, broken or both because of this simple problem. The fix is trivial, doesn't affect whether you can use it for testing codecs (if that's the "context" you're talking about), and will make the project better. What possible reason could there be to refuse to even say "yes, we'll accept a patch for this", other than ego?

[JohnAStebbins]

Wouldn't it be easier to just incrementally grow the tool you already have?

You missed the point entirely. The tool does grow incrementally. Just not in the direction you are asking for. It behaves the way it does for reasons you don't find useful for your purposes. The powers that be don't want it's behaviour changed for those reasons. So the alternative that is left is to create a different tool targeted for a different purpose.

If the CLI tool is intentionally useless because it's just for testing, why do you offer compiled binaries for download from the website without warning users?

Many users find it useful. In fact you are in the vast minority if you wanted to take a head count.

The fix is trivial, doesn't affect whether you can use it for testing codecs

Incorrect. ctrl-c would cause a test to register as a failure when it is not.

[fonkprop]

Incorrect. ctrl-c would cause a test to register as a failure when it is not.

Riiiight. Because you guys are of course totally unable to inspect the return code to see if it was a SIGINT or not. Look, I've reported the bug, explained why it's a problem, and tried to reason the tardy through the logic behind error return codes. Hopefully the next poor shmo that hits this problem will find this thread, and not bother to go further. I'll leave you geniuses to discuss this amongst yourselves, mkay?

[jbrjake]

Err...no reason to antagonize John. He was pretty much agreeing with you except for the one technical point you were glossing over.

Thank you for the dd example, but I don't think its model is really applicable for HB. Please direct me to the relevant line numbers if I'm missing something. It just seems to be closing the file handle and, when necessary, the stdout stream. That isn't all HB is doing when you hit ctrl-c. It has ongoing muxing and syncing threads which have to block, flush, and close. It has a linked list of work objects like the audio and video coders that have to closed. The input demuxer has to be closed when ending early. There are fifos that need to be closed, plus the configuration structs for the different streams. Filters have to be closed. The buffer pool has to be freed. The control objects need to be disposed of too.

A lot of these are handled using custom wrappers that HB maintains, like hb_buffer_pool_free() and the individualized closing functions in each work object's code base. There are many, many opportunities for things to break. It is a complex process with many steps. In light of this, it is quite useful to know when the process is working and when it is not. When it is working, cancelling an encode does not lead to an error exit status. When it is not working, cancelling an encode leads to an error exit status.

dd's developers can assume that close() is going to work just fine all the time. It's a standard function. We cannot assume that a problem hasn't been introduced in, say, a new filter's custom closing function. We need to know when that produces an error and when it is succeeding.

Remember, sysexits.h, the only exit code list that even attempts to be authoritative across applications, defines 0 as "successful termination."

[kasperta]

In an effort to be pragmatic about this issue (which doesn't really matter much to me): have anyone considered accepting a patch from fonkprop, adding an argument to change the behaviour of the exit status?

I would think this is acceptable in a piece of test software?