Disabling stdout error messages

[dancooke]

Would it be possible to make htslib's error reporting optional, perhaps in the form of a preprocessor directive? I'm writing an application which makes heavy use of htslib, and would like to maintain a consistent error reporting style, but this is not possible because htslib keeps polluting my own error reporting, e.g.:

htsFile* file = sam_open("fake.bam", "r");
[E::hts_open_format] fail to open file 'fake.bam'

This error message is simply unnecessary; the caller is quite capable of checking the return value.

In cases where there is more than one failure condition (e.g. in bam_hdr_read), I think it's preferable to have an explicit method for diagnosing the problem:

bam_hdr_t* header = sam_hdr_read(file);
if (!header) {
    char* problem = diagnose_sam_hdr_error(file);
}

These two approaches would not affect existing code that relies on htslib error reporting, but also afford new library users greater flexibility.

[daviesrob]

Most (but sadly not all yet) of htslib's error reporting checks the global variable hts_verbose to see if the message should be printed. For example, in hts_open_format we have:

error:
    if (hts_verbose >= 2)
        fprintf(stderr, "[E::%s] fail to open file '%s'\n", __func__, fn);

So setting hts_verbose to 1 or less should make this message go away.

[jmarshall]

Those two in hts_open() et al in particular drive me mad. You end up with messages like

$ samtools view bork.bam
[E::hts_open_format] fail to open file 'bork.bam'
samtools view: failed to open "bork.bam" for reading: No such file or directory

and the htslib message is the less informative one. I've had a change stashed for a while to change these to if (hts_verbose >= 4) so they don't appear with the default hts_verbose (3), but actually I'd rather verify that all hts_open() et al invocations in samtools/bcftools/htslib print their own messages and then nuke these two fprintfs within hts_open() et al.

[anderskaplan]

Would you be interested in a contrib (pull request) that solves this issue? I'm looking for a good open source project to get involved in, and this could be a good starting point.

[daviesrob]

@anderskaplan You're welcome to try, but I should warn you that it might not be the ideal thing to start on. You would end up touching most of the files in htslib. That tends to result in big pull requests that need lots of revision and may end up not being merged. It would likely be best done in stages.

What it needs is:

• An enumerated type hts_log_level with names like HTS_LOG_ERROR, HTS_LOG_WARN, HTS_LOG_DEBUG and values roughly matching the current use of hts_verbose.

• A varargs function with a signature like hts_log(hts_log_level level, func, format, ...) that checks for hts_verbose >= level and then prints the "[E::func] ..." message if true.

• Replace all the fprintf(stderr, ...) calls with ones that use the function above.

I get closer to doing this myself every time I type in:

if (hts_verbose >= 1) {
   fprintf(stderr, "[E::%s] Something bad happened: %s\n", __func__, strerror(errno));
}

[anderskaplan]

Another thing that I don't think is appropriate for a library is to call exit() and abort() on its own. I'd rather have it return with an error, so that the calling application gets a chance to at least clean up. Any opinions about that?

[AndreasHeger]

Really great work on the logging, and thanks, that would help pysam a lot.

[jkbonfield]

@anderskaplan I fully agree that abort() is a bad thing for a library. There are a few places it may be deemed acceptable (coding errors rather than data/system driven errors), but we have too many that are simply a quick and dirty alternative to proper error handling.