Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Andreas Tille]

On Thu, Dec 14, 2017 at 02:30:53PM +0100, Mattia Rizzolo wrote:
> 
> ACK, should be all good to go now.

Thanks for the careful review.  That's really appreciated

   Andreas.

-- 
http://fam-tille.de

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Mattia Rizzolo]

On Thu, Dec 14, 2017 at 02:26:46PM +0100, Andreas Tille wrote:
> I've tested a local build and the means of
> 765419270800864fd29cea90db233a60cd46aea8 can all be droped.  I pushed
> two commits - hopefully better documented than before.

ACK, should be all good to go now.

-- 
regards,
Mattia Rizzolo

GPG Key: 66AE 2B4A FCCF 3F52 DA18  4D18 4B04 3FCD B944 4540  .''`.
more about me:  https://mapreri.org : :'  :
Launchpad user: https://launchpad.net/~mapreri  `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia  `-


signature.asc
Description: PGP signature

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Andreas Tille]

Hi Mattia,

On Thu, Dec 14, 2017 at 01:31:35PM +0100, Mattia Rizzolo wrote:
> On Wed, Dec 13, 2017 at 04:49:36PM +0100, Andreas Tille wrote:
> > Thanks for your careful checking.  Would you mind just commiting your
> > prefered solution to make sure I've correctly understood what you mean?
> > (There is no real policy in the med team here.)
> 
> I could do that, but I'm unsure as to why you did a thing.
> I'm looking at commit 765419270800864fd29cea90db233a60cd46aea8, where
> with a simple message "install htslib*.mk files that are used in
> bcftools" you did so many changes, including adding a dependency and
> installing a bunch of links.  The problem is the first link of that
> list.  The commit nor the changelog explains the reason of those
> changes, so I'd rather not remove something that is there for a
> reason...

Sorry for the sloppy commit messages.
 
> Then, now that the htslib*.mk are not installed anymore
> (commit 5fedaeab7241efc00f5d814ebb67f884d1c62e38) so *maybe* all the
> changes that were done in the same commit could be reverted as well.

I've tested a local build and the means of
765419270800864fd29cea90db233a60cd46aea8 can all be droped.  I pushed
two commits - hopefully better documented than before.

Sorry for the confusion

  Andreas.

-- 
http://fam-tille.de

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Mattia Rizzolo]

On Wed, Dec 13, 2017 at 04:49:36PM +0100, Andreas Tille wrote:
> Thanks for your careful checking.  Would you mind just commiting your
> prefered solution to make sure I've correctly understood what you mean?
> (There is no real policy in the med team here.)

I could do that, but I'm unsure as to why you did a thing.
I'm looking at commit 765419270800864fd29cea90db233a60cd46aea8, where
with a simple message "install htslib*.mk files that are used in
bcftools" you did so many changes, including adding a dependency and
installing a bunch of links.  The problem is the first link of that
list.  The commit nor the changelog explains the reason of those
changes, so I'd rather not remove something that is there for a
reason...

Then, now that the htslib*.mk are not installed anymore
(commit 5fedaeab7241efc00f5d814ebb67f884d1c62e38) so *maybe* all the
changes that were done in the same commit could be reverted as well.
But who knows…

-- 
regards,
Mattia Rizzolo

GPG Key: 66AE 2B4A FCCF 3F52 DA18  4D18 4B04 3FCD B944 4540  .''`.
more about me:  https://mapreri.org : :'  :
Launchpad user: https://launchpad.net/~mapreri  `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia  `-


signature.asc
Description: PGP signature

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Andreas Tille]

Hi Mattia,

On Wed, Dec 13, 2017 at 04:40:46PM +0100, Mattia Rizzolo wrote:
> 
> One thing: I noticed that in htslib's tracker page an important
> multiarch issue is reported: libhts-dev is marked ma:same, but there is
> a static library in a non-multiarch path.
> The options to fix it are:
> * don't build the static lib at all
> * move the static lib to a multiarch path
> * remove the ma:same notation
> (I prefer them in that order, personally, but IIUC static libraries are
> kind of liked by science/med people; icbw of course)

Thanks for your careful checking.  Would you mind just commiting your
prefered solution to make sure I've correctly understood what you mean?
(There is no real policy in the med team here.)

Thanks a lot

   Andreas.

-- 
http://fam-tille.de

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Andreas Tille]

Hi Mattia,

On Wed, Dec 13, 2017 at 12:53:48PM +0100, Mattia Rizzolo wrote:
> > What is the argument in favour of considering clearly unsupported
> > undocumented internal functions to be part of a library's interface
> > just because symbols are visible — in the binary but not the headers?
> 
> I've been convinced off list to not being so uptight in technicalities.
> My argument would be only about definitions and being "academically
> right", which is clearly way too overzealous sometimes...
> Let's just say those functions were private and are not supposed to be
> used by anything, so it's "fine enough" to just drop them from the
> symbols list.

To come back to the initial discussion:  You would agree that uploading
to unstable is fine?  The reason why I've choosen experimental initially
was also that we have lots of packages depending from python-pysam which
sometimes is lagging behind samtools.  I just verified that there is
a new python-pysam version which says in the NEWS file:

  This release wraps htslib/samtools/bcftools versions 1.6.0 and
  contains a series of bugfixes.

So I would go on an move the packages htslib, samtools and bcftools from
experimental to unstable and upload python-pysam 0.13.0 if nobody
insists.

Kind regards

 Andreas.

-- 
http://fam-tille.de

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Mattia Rizzolo]

On Tue, Dec 12, 2017 at 11:17:22PM +, John Marshall wrote:
> But in short, upstream's guiding principle is this: API and ABI are
> tightly related. If functions that are not declared within htslib/*.h
> (therefore are not part of HTSlib's documented public API) are
> changed, we do not consider that to be an ABI change. Third-party
> code should not be calling such functions, end of story.
> 
> To use debug_vbuf() or hopen_json_redirect() or these other functions,
> authors of client code would have to search for them in the internal
> part of HTSlib's source code and copy suitable declarations into their
> own code in order to call them or otherwise use them. We do not
> consider that to be a supported use of the HTSlib library, and I
> think you would struggle to find any other library that supported
> such use. If such things were supported, how could library authors
> make changes to the behaviour of any (non-static) internal functions
> ever [1]?

Changing the behaviour is totally orthogonal to breaking ABI, in my
opinion.  Breaking the ABI means an application dynamically linked to it
won't even start.

> This argument applies to all of the functions mentioned in your email.
> 
> What is the argument in favour of considering clearly unsupported
> undocumented internal functions to be part of a library's interface
> just because symbols are visible — in the binary but not the headers?

I've been convinced off list to not being so uptight in technicalities.
My argument would be only about definitions and being "academically
right", which is clearly way too overzealous sometimes...
Let's just say those functions were private and are not supposed to be
used by anything, so it's "fine enough" to just drop them from the
symbols list.

> [1] Yes, we're aware of non-portable visibility declarations, and
> have historically had limited interest in taking up that maintenance
> burden in HTSlib.

I see. pity :)

-- 
regards,
Mattia Rizzolo

GPG Key: 66AE 2B4A FCCF 3F52 DA18  4D18 4B04 3FCD B944 4540  .''`.
more about me:  https://mapreri.org : :'  :
Launchpad user: https://launchpad.net/~mapreri  `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia  `-


signature.asc
Description: PGP signature

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[John Marshall]

On 13 Dec 2017, at 10:47, Mattia Rizzolo  wrote:
> These three symbols are from fuctions that are clearly aimed at debug.
> The functions do dumb fprintf() of some data structures to stderr,
> clearly something that nobody would ever use in a production system.
> The fuctions themselves are still there, hidden by some #ifdef.  I
> reckon they were also never intended to be so widely available.

Indeed.

> Nonetheless, that's an ABI break, and for the sake of peace of mind,
> I'd re-enable them until the next SONAME bump (or anyway, the next
> defined ABI break), even if they hopefully are used by nothing.

Why? Don't people who go far out of their way (see below) to use such internal 
functions deserve what they get?

Even if the function signature were unchanged, what expectation do third 
parties have that an undocumented internal function still has the behaviour 
they're expecting in an updated library version?

>> hopen_json_redirect
> 
> This was removed (well, replaced by hopen_htsget_redirect) with:
> https://github.com/samtools/htslib/commit/7cdd5749e8054527912df184be87ac7663b01115
> IMHO, that's a clear ABI break, except for the fact that happened in a
> file that is clearly something that is part of some private internal.
> All the other functions from the same file are static, I wonder why that
> one is not.

Because it's the external entry point that is called by code in other source 
files within HTSlib.

> I'd try to engage upstream for this, they were very helpfully in the
> past itarations and in pointing out our packaging faults, I hope they
> can be of help again.

Me, I have limited time for this this week, but would hope to be able to look 
at the changes you folks have made next week sometime.

But in short, upstream's guiding principle is this: API and ABI are tightly 
related. If functions that are not declared within htslib/*.h (therefore are 
not part of HTSlib's documented public API) are changed, we do not consider 
that to be an ABI change. Third-party code should not be calling such 
functions, end of story.

To use debug_vbuf() or hopen_json_redirect() or these other functions, authors 
of client code would have to search for them in the internal part of HTSlib's 
source code and copy suitable declarations into their own code in order to call 
them or otherwise use them. We do not consider that to be a supported use of 
the HTSlib library, and I think you would struggle to find any other library 
that supported such use. If such things were supported, how could library 
authors make changes to the behaviour of any (non-static) internal functions 
ever [1]?

This argument applies to all of the functions mentioned in your email.

What is the argument in favour of considering clearly unsupported undocumented 
internal functions to be part of a library's interface just because symbols are 
visible — in the binary but not the headers?

John

[1] Yes, we're aware of non-portable visibility declarations, and have 
historically had limited interest in taking up that maintenance burden in 
HTSlib.

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Mattia Rizzolo]

So, I had a closer look at it:

On Mon, Dec 11, 2017 at 05:23:19PM +0100, Mattia Rizzolo wrote:
> 1) you happily removed symbols: why so?  removing symbols is a ABI
> break, and that would usually call for a SONAME bump that upstream
> didn't do.  Those things need to be investigated singularly, what were
>  cram_stats_dump
>  debug_vbuf
>  debug_vsets

These three symbols are from fuctions that are clearly aimed at debug.
The functions do dumb fprintf() of some data structures to stderr,
clearly something that nobody would ever use in a production system.
The fuctions themselves are still there, hidden by some #ifdef.  I
reckon they were also never intended to be so widely available.
Nonetheless, that's an ABI break, and for the sake of peace of mind,
I'd re-enable them until the next SONAME bump (or anyway, the next
defined ABI break), even if they hopefully are used by nothing.

>  hopen_json_redirect

This was removed (well, replaced by hopen_htsget_redirect) with:
https://github.com/samtools/htslib/commit/7cdd5749e8054527912df184be87ac7663b01115
IMHO, that's a clear ABI break, except for the fact that happened in a
file that is clearly something that is part of some private internal.
All the other functions from the same file are static, I wonder why that
one is not.
I'd try to engage upstream for this, they were very helpfully in the
past itarations and in pointing out our packaging faults, I hope they
can be of help again.

I'm therefore CCing that guy from upstream that engaged with us the last
month.



PS: John Marshall: I hope you consider what we did to solve the issues
you pointed out with #881170 acceptable for you.  Again, please raise
your concerns if anything else is amiss!

-- 
regards,
Mattia Rizzolo

GPG Key: 66AE 2B4A FCCF 3F52 DA18  4D18 4B04 3FCD B944 4540  .''`.
more about me:  https://mapreri.org : :'  :
Launchpad user: https://launchpad.net/~mapreri  `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia  `-


signature.asc
Description: PGP signature

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Andreas Tille]

Hi Mattia,

On Mon, Dec 11, 2017 at 06:29:26PM +0100, Mattia Rizzolo wrote:
> On Mon, Dec 11, 2017 at 05:44:53PM +0100, Andreas Tille wrote:
> > Sorry for not beeing more verbose.  This was the result of the patch
> > I've got when trying to build the package *including* the symbols.  :-(
> > These were marked as "# MISSING #".
> > 
> > Feel free to try your luck without removing ...
> 
> Sure, I'm just saying that that "patch" is not supposed to be blindly
> applied.
> If it were then we wouldn't need to manually maintain the file...

I need to admit that any better fix would be more than welcome.
Unfortunately I had no better clue. :-(

Kind regards

 Andreas.

-- 
http://fam-tille.de

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Mattia Rizzolo]

On Mon, Dec 11, 2017 at 05:44:53PM +0100, Andreas Tille wrote:
> Sorry for not beeing more verbose.  This was the result of the patch
> I've got when trying to build the package *including* the symbols.  :-(
> These were marked as "# MISSING #".
> 
> Feel free to try your luck without removing ...

Sure, I'm just saying that that "patch" is not supposed to be blindly
applied.
If it were then we wouldn't need to manually maintain the file...

-- 
regards,
Mattia Rizzolo

GPG Key: 66AE 2B4A FCCF 3F52 DA18  4D18 4B04 3FCD B944 4540  .''`.
more about me:  https://mapreri.org : :'  :
Launchpad user: https://launchpad.net/~mapreri  `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia  `-


signature.asc
Description: PGP signature

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Andreas Tille]

On Mon, Dec 11, 2017 at 05:23:21PM +0100, Mattia Rizzolo wrote:
> 
> Although, looking at
> https://anonscm.debian.org/cgit/debian-med/htslib.git/commit/?id=ca55c40f2a3154004d6e55eac4bdc2cdffa413b3
> I noticed that:
> 
> 1) you happily removed symbols: why so?  removing symbols is a ABI
> break, and that would usually call for a SONAME bump that upstream
> didn't do.  Those things need to be investigated singularly, what were
>  cram_stats_dump
>  debug_vbuf
>  debug_vsets
>  hopen_json_redirect
> and why were they removed.  Just happily removing them from the .symbols
> file is NOT acceptable.
> If you did such research at the very least I'd expect to see an
> explanation in the commit message

Sorry for not beeing more verbose.  This was the result of the patch
I've got when trying to build the package *including* the symbols.  :-(
These were marked as "# MISSING #".

Feel free to try your luck without removing ... 

> 2) the version used in the new symbols should be '1.6' not '1.6-1'

Fixed in Git.

Kind regards and thanks a lot for checking

   Andreas.

-- 
http://fam-tille.de

Re: Uploaded new version 1.6 of htslib, samtools & bcftools to experimental - any known issues with reverse dependencies

[Mattia Rizzolo]

On Mon, Dec 11, 2017 at 04:13:39PM +0100, Andreas Tille wrote:
> since we frequently observed issues with incompatibilities of samtools I
> uploaded the new version to experimental first.  Strictly speaking we
> should check whether a transition is needed.

Of course a transition (given the definition used by the release team)
is not needed given that there are no versioned Breaks or SONAME bumps
involved here.
Also, now htslib has a symbols file, so there is no need for shlibs
bumps or so.

Although, looking at
https://anonscm.debian.org/cgit/debian-med/htslib.git/commit/?id=ca55c40f2a3154004d6e55eac4bdc2cdffa413b3
I noticed that:

1) you happily removed symbols: why so?  removing symbols is a ABI
break, and that would usually call for a SONAME bump that upstream
didn't do.  Those things need to be investigated singularly, what were
 cram_stats_dump
 debug_vbuf
 debug_vsets
 hopen_json_redirect
and why were they removed.  Just happily removing them from the .symbols
file is NOT acceptable.
If you did such research at the very least I'd expect to see an
explanation in the commit message
2) the version used in the new symbols should be '1.6' not '1.6-1'

-- 
regards,
Mattia Rizzolo

GPG Key: 66AE 2B4A FCCF 3F52 DA18  4D18 4B04 3FCD B944 4540  .''`.
more about me:  https://mapreri.org : :'  :
Launchpad user: https://launchpad.net/~mapreri  `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia  `-


signature.asc
Description: PGP signature