Huzzah. This also deserves a NEWS entry, and there are some knetfile/knet_win32 references in test/header_syms.pl and test/maintainer/check_spaces.pl that could go.

I guess some will consider the removal of htslib/knetfile.h and the functions declared there to be a removal of vital public API functions. Pedantically correct, but… sigh… noone should be using these functions directly.

So either this perhaps warrants a soversion bump, or perhaps as a transition before removal during a later soversion bump in a future release, replace the implementations of knet_open() etc with dummy versions that just call through to hopen() (and hence newly fail if libcurl was not configured — tough!) and have them maintain knetFile::offset and maybe knetFile::fd I guess (for the sake of the knet_tell(), knet_fileno() macros)…

That's a good point - knetfile.h was a public header.

Maybe it should have stub functions (static inlines?).

Annoyingly, in the absence of a soversion bump they would need to be real symbols in libhts.so.

Which would mean keeping this PR as a draft to be applied in the eventual happy future, and making a new smaller one that basically keeps the infrastructure but replaces all the function bodies within knetfile.c. Alas…

It appears to be trivial to get most of the ABI to still exist, eg:

typedef struct knetFile_s knetFile;
knetFile *knet_open(const char *fn, const char *mode) {
    return (knetFile *)hopen(fn, mode);
}

knetFile *knet_dopen(int fd, const char *mode) {
    return (knetFile *)hdopen(fd, mode);
}

ssize_t knet_read(knetFile *fp, void *buf, size_t len) {
    return hread((hFILE *)fp, buf, len);
}

off_t knet_seek(knetFile *fp, off_t off, int whence) {
    return hseek((hFILE *)fp, off, whence);
}

int knet_close(knetFile *fp) {
    return hclose((hFILE *)fp);
}

API though is a bit less clear due to the hfile_fileno macro. Frankly it looks to be never used outside of ancient copies of bcftools. The macro for knet_tell would need changing too, which is the fly in the ointment as far as ABI goes. It's trivial to retain API compatibility by simply changing it to htell. I could keep track of offset internally of course, but that means allocating new structures etc. Pondering...

I guess some will consider the removal of htslib/knetfile.h and the functions declared there to be a removal of vital public API functions. Pedantically correct, but… sigh… noone should be using these functions directly.

They appear to be endemic in older bgzf implementations. E.g. see

https://github.com/search?q=knet_dopen+NOT+filename%3Aknetfile.h+NOT+filename%3Akurl.h+NOT+filename%3Amain.c+NOT+filename%3Abgzf.h+NOT+filename%3Aknetfile.c&type=code

However the packages which copied in their own internal copy of bgzf.c have mostly also copied in knet.c (why would you copy one without the other if it has a dependency?). The only package I saw that didn't was EMBOSS, but then the bgzf.c is #ifdefed around _USE_KNETFILE which wouldn't be set anyway.

This cargo-cult meme really makes github searching a royal PITA! It also means people won't get fixes and "problems" are endemic across bioinformatics. :(

I couldn't find much else in usage though. That, kurl obviously, and ancient bcftools copies (again if you've gone to the effort of bundling the entire bcftools source into your package then you'll have also bundled the rest of it too).

Simplest solution might be to remove its ability to read anything other than local files, which would make the implementation trivial, and also mark the whole thing as deprecated using HTS_DEPRECATED. Or possibly even refuse to open files at all, which would make the stub even simpler.

Ok, so this second commit undoes some things in the first, so it needs squashing, but is left as-is for now so reviewers can see the history.

• htslib/knetfile.h now exists again for API compatibility. I'm still open to removing this (it's not needed by any of this code), which will remove knet from the API without affecting ABI. It depends on whether we wish to give warning of impending demise or not. (It has HTS_DEPRECATED markers).
• hfile.c now has a dummy set of knet_* functions which are simply wrappers around hopen/hread etc. They manually track offset so the knet_tell macro still works (ABI compat). NB: it doesn't include knetfile.h so it's just a hacked duplicated. Perhaps I could move the hf structure member to in-situ of another char* so the structure size doesn't change incase people are using arrays of knetFiles, but I suspect that's unnecessary.
• The windows knet functions have gone, but they weren't listed using HTS_EXPORT so I assume they were not publically callable anyway.
• The knet_fileno macro is hideous to implement for curl (not sure even if possible), but the only uses I could find of it were immediately followed by fstat so I made it work for local files only and it returns -1 otherwise.

I've validated this using this tiny program:

#include <stdio.h>
#include "htslib/knetfile.h"

int main(int argc, char **argv) {
    char dat[100];

    knetFile *fp = knet_open(argv[1], "r");
    if (!fp) {
	perror(argv[1]);
	return 1;
    }
    knet_seek(fp, 15, SEEK_SET);
    int x = knet_read(fp, dat, 50);
    printf("FD %d: Read '%.*s'\n", knet_fileno(fp), x, dat);
    printf("Now at pos %ld\n", (long)knet_tell(fp));
    return knet_close(fp);
}

I built this against htslib 1.11 headers, and have switched dynamic library by use of LD_LIBRARY_PATH. I can confirm it works as expected.

Thanks. Don't know what went on with that stray "l". I previously had commented out the deprecated lines as it made my testing a pain. I guess I finger slipped removing the ;//.

Comment in libcurl.c has been nuked. I left it there before because I wasn't sure which bit to delete (is the RFC comment for knet or for the whole idea of full pathnames in general?), but I'm sure curl must be pretty compliant and if not I doubt we need to be spelling it out.

The RFC comment was about using full pathnames. RFC1738 says you should CWD through each directory, curl did by default but knet did not, which made curl quite a bit slower in comparison.

Git blame suggests you added the original comment in commit 5bd5b95.

Fair enough. I reinstated that bit of comment again then.