Bug 1759940 - Review Request: htslib - C library for high-throughput sequencing data formats
Summary: Review Request: htslib - C library for high-throughput sequencing data formats
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robert-André Mauchin 🐧
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1131121 1738176
TreeView+ depends on / blocked
 
Reported: 2019-10-09 13:05 UTC by Jun Aruga
Modified: 2019-11-05 05:28 UTC (History)
5 users (show)

Fixed In Version: htslib-1.9-3.fc32
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-11-05 03:56:53 UTC
Type: ---
Embargoed:
zebob.m: fedora-review+


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 1326504 0 unspecified CLOSED Review Request: htslib - C library for high-throughput sequencing data formats (required for `samtools`) 2021-02-22 00:41:40 UTC

Description Jun Aruga 2019-10-09 13:05:21 UTC
Spec URL: https://raw.githubusercontent.com/junaruga/htslib-pkg/hotfix/review/htslib.spec
SRPM URL: https://github.com/junaruga/htslib-pkg/blob/hotfix/review/htslib-1.9-1.fc32.src.rpm?raw=true
Description: C library for high-throughput sequencing data formats
Fedora Account System Username: jaruga

Comment 1 Jun Aruga 2019-10-09 13:13:38 UTC
This is an inheritance ticket from https://bugzilla.redhat.com/show_bug.cgi?id=1326504 .
The ticket is created to request the Fedora dist-git repository by my account.

The Spec URL and SRPM URL and the contents are same as the last ones of the ticket
https://bugzilla.redhat.com/show_bug.cgi?id=1326504#c26

* Koji scratch build: ok https://koji.fedoraproject.org/koji/taskinfo?taskID=38010659
* Test to install RPMs: ok
* rpmlint: ok

Robert, can I ask you to change "fedora-review+" again like following comment?
https://bugzilla.redhat.com/show_bug.cgi?id=1326504#c34

Comment 2 Jun Aruga 2019-10-09 13:17:59 UTC
*** Bug 1326504 has been marked as a duplicate of this bug. ***

Comment 3 Zbigniew Jędrzejewski-Szmek 2019-10-09 14:00:03 UTC
Review was done in https://bugzilla.redhat.com/show_bug.cgi?id=1326504#c34.

Comment 4 Jun Aruga 2019-10-09 15:26:50 UTC
Zbigniew, thanks for your review again!
Now I am requesting the repository.

```
$ fedpkg request-repo htslib 1759940
https://pagure.io/releng/fedora-scm-requests/issue/18058
```

Comment 5 Robert-André Mauchin 🐧 2019-10-09 15:49:44 UTC
Re-request now.

Comment 6 Jun Aruga 2019-10-09 16:07:29 UTC
Sure, I did it now again.

```
$ fedpkg request-repo htslib 1759940
https://pagure.io/releng/fedora-scm-requests/issue/18061
```

Comment 7 Gwyn Ciesla 2019-10-09 16:32:37 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/htslib

Comment 8 Jun Aruga 2019-10-09 16:45:11 UTC
Thank you, Robert and Gwyn! 

Robert, I appreciate your continuous work.
Do you know why comment 4's "fedpkg request-repo" is failed, and you needed to update "fedora-review+"?

Now I am requesting the branches.

```
$ fedpkg request-branch --repo htslib f31
https://pagure.io/releng/fedora-scm-requests/issue/18062
$ fedpkg request-branch --repo htslib f30
https://pagure.io/releng/fedora-scm-requests/issue/18063
$ fedpkg request-branch --repo htslib f29
https://pagure.io/releng/fedora-scm-requests/issue/18064
```

Comment 9 Robert-André Mauchin 🐧 2019-10-09 17:03:02 UTC
(In reply to Jun Aruga from comment #8)
> Thank you, Robert and Gwyn! 
> 
> Robert, I appreciate your continuous work.
> Do you know why comment 4's "fedpkg request-repo" is failed, and you needed
> to update "fedora-review+"?

Because the Assignee was me and the person who set the fedora-review+ flag was Zbigniew Jędrzejewski-Szmek: both need to be the same in order to fedpkg request-repo to work.

Comment 10 Zbigniew Jędrzejewski-Szmek 2019-10-09 17:40:38 UTC
Sorry, I didn't know this, and I didn't want to take credit for work I didn't do.

Comment 11 John Marshall 2019-10-14 11:05:34 UTC
HTSlib uses the usual configure/make/make install build system, and falls back to a basic configuration if you didn't run configure. So your spec file should use configure, via %configure as appropriate.

In particular Fedora should provide a fully-featured HTSlib build by configuring with

    ./configure --enable-plugins --with-plugin-path='/usr/local/libexec/htslib:$(plugindir)' --enable-libcurl --enable-gcs --enable-s3

See HTSlib's INSTALL file for details.

This will activate HTSlib's plugin mechanism so that the Fedora-supplied libhts.so will be able to use any other file access plugins that the user may have installed, and will make Fedora's libhts.so use libcurl (thus variously incurring dependencies on libcurl and libcurl-devel) to enable HTTPS and related file access methods. Your current build can do basic FTP and HTTP but not HTTPS, which means it is of rapidly decreasing usefulness.


The file format man pages (sam.5 etc) are aimed at users of tools eg samtools, not only at developers using the API. So IMHO they would fit better in the htslib-tools subpackage alongside the other manual pages.

Comment 12 Jun Aruga 2019-10-14 11:30:50 UTC
Hi John
Thanks for your feedback!

> In particular Fedora should provide a fully-featured HTSlib build by
> configuring with
> 
>     ./configure --enable-plugins
> --with-plugin-path='/usr/local/libexec/htslib:$(plugindir)' --enable-libcurl
> --enable-gcs --enable-s3

You are right. Let me change the spec file later.

> The file format man pages (sam.5 etc) are aimed at users of tools eg
> samtools, not only at developers using the API. So IMHO they would fit
> better in the htslib-tools subpackage alongside the other manual pages.

Yeah, the man pages are for user's document. These are main document. I feel it might be better place than htslib-devel.
How other foolib packages dealing with it? Later I need to check it.

```
%{_mandir}/man5/faidx.5*
%{_mandir}/man5/sam.5*
%{_mandir}/man5/vcf.5*
```

Comment 13 Jun Aruga 2019-10-14 11:36:33 UTC
Robert-André and Zbigniew,
Sure, thanks for your help, and also for your comment on devel@ mailing list about htslib.

I am still reading it. It's useful for foolib packagers :)

Review swap (htslib)
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/WHZBVT3Y7TKYAEK6EXEY6L77W42T7LJX/#FKY4NG3TLUVHPZXS6HXGXO6EPZZOLSRB

Comment 14 Jun Aruga 2019-10-21 21:04:27 UTC
Hi John,

> You are right. Let me change the spec file later.

I sent a pull-request to Fedora repository about your configure script's suggestion.
Could you review or comment below page? Thanks.
https://src.fedoraproject.org/rpms/htslib/pull-request/1

> The file format man pages (sam.5 etc) are aimed at users of tools eg
> samtools, not only at developers using the API. So IMHO they would fit
> better in the htslib-tools subpackage alongside the other manual pages.

Currently htslib-tools sub package is for binary tools such as bgzip, htsfile and tabix binary commands.
I am not comforable to add the file format man page files (= man5 [1]) to the htslib-tools.

Right now I am checking other spec file's situation for man5 files.

* libreport.spec
  main package
* libbind.spec
  devel sub package
* libguestfs.spec
  tools-c sub package
* libpng.spec
  main package
* libselinux.spec
  utils sub package

[1] https://linux.die.net/man/

Comment 15 John Marshall 2019-10-21 22:46:07 UTC
> I sent a pull-request to Fedora repository about your configure script's suggestion.
> Could you review or comment below page? Thanks.
> https://src.fedoraproject.org/rpms/htslib/pull-request/1

1. The plugins are loaded when libhts.so.2 is used. Hence they should be in the main package, not the -devel subpackage.

2. In %files, I suggest using %{_libexecdir}/%{name}/hfile_*.so rather than explicitly listing the three that currently exist.

3. You are building from an htslib release tarball, which already has configure built. So there is no need to run autoheader/autoconf or to build-require autoconf.

>> The file format man pages (sam.5 etc) are aimed at users of tools eg
>> samtools, not only at developers using the API. So IMHO they would fit
>> better in the htslib-tools subpackage alongside the other manual pages.
>
> Currently htslib-tools sub package is for binary tools such as bgzip, htsfile
> and tabix binary commands.
> I am not comforable to add the file format man page files (= man5 [1]) to the htslib-tools.

They are for users, not just developers, so it is not appropriate to put them in the -devel package. If you don't want to put them in the -tools package, then put them in the main package -- in fact, that's probably an even better fit as some users will fool-hardily omit to install htslib-tools.

> Right now I am checking other spec file's situation for man5 files.
>
> * libreport.spec
>  main package
> * libbind.spec
>  devel sub package

That is incorrect -- libbind.spec puts the majority of its man pages in the main package.

> * libpng.spec
>  main package

So your survey of other spec files reinforces putting htslib's *.5 in the main package.

    John

Comment 16 Jun Aruga 2019-10-22 20:15:49 UTC
> 2. In %files, I suggest using %{_libexecdir}/%{name}/hfile_*.so rather than explicitly listing the three that currently exist.

According to following document, so files in %files should not be listed by using a glob. The example 's pattern "libfoo.so*" is different from the pattern "hfile_*.so".
But I feel I need to be conscious for each so file.
So, I would prefer the explicitly listing way.

> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files
> Shared libraries installed directly into %{_libdir} SHOULD NOT be listed in the %files section of the spec by using a glob in a way that conceals important parts of the file name (e.g. libfoo.so*), since changes to the SONAME also result in a changed file name in most cases.

Other parts you mentioned make sense for me. I updated it following your suggestions.
I rebased the pull-request, including the man5 pages to the main package.
Could you review again?
https://src.fedoraproject.org/rpms/htslib/pull-request/1

Thanks for your review.

Comment 17 Jun Aruga 2019-10-24 18:05:04 UTC
> Could you review again?
> https://src.fedoraproject.org/rpms/htslib/pull-request/1

I merged and built it.
https://koji.fedoraproject.org/koji/taskinfo?taskID=38532540

Comment 18 Fedora Update System 2019-10-24 18:44:35 UTC
FEDORA-2019-cdb212f45d has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2019-cdb212f45d

Comment 19 Fedora Update System 2019-10-24 18:45:46 UTC
FEDORA-2019-21ca0b0c3c has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-21ca0b0c3c

Comment 20 Fedora Update System 2019-10-24 18:46:50 UTC
FEDORA-2019-36fd2ef2e1 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-36fd2ef2e1

Comment 21 Fedora Update System 2019-10-25 19:34:43 UTC
htslib-1.9-2.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-21ca0b0c3c

Comment 22 John Marshall 2019-10-25 21:18:33 UTC
> According to following document, so files in %files should not be listed by using a glob.
>
>> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files
>> Shared libraries installed directly into %{_libdir} SHOULD NOT be listed in the %files section of the spec by using a glob in a way that conceals important parts of the file name (e.g. libfoo.so*), since changes to the SONAME also result in a changed file name in most cases.

These plugin files are not shared libraries, are not installed into libdir, do not have SONAMEs, and the glob does not conceal parts that are important in the way mentioned. So this paragraph does not apply. But you should do as you see fit.

However there is a bug here. The htslib main package creates the libexecdir/htslib directory so needs to package the directory itself so that it will be deleted on uninstall. So what this really wants is

%files
%{_libexecdir}/%{name}

or equivalent.

Comment 23 Fedora Update System 2019-10-25 21:27:07 UTC
htslib-1.9-2.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-36fd2ef2e1

Comment 24 Fedora Update System 2019-10-26 15:31:55 UTC
htslib-1.9-2.fc31 has been pushed to the Fedora 31 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-cdb212f45d

Comment 25 Jun Aruga 2019-10-27 20:53:35 UTC
(In reply to John Marshall from comment #22)
> > According to following document, so files in %files should not be listed by using a glob.
> >
> >> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files
> >> Shared libraries installed directly into %{_libdir} SHOULD NOT be listed in the %files section of the spec by using a glob in a way that conceals important parts of the file name (e.g. libfoo.so*), since changes to the SONAME also result in a changed file name in most cases.
> 
> These plugin files are not shared libraries, are not installed into libdir,
> do not have SONAMEs, and the glob does not conceal parts that are important
> in the way mentioned. So this paragraph does not apply. But you should do as
> you see fit.

Sure, let me do without a glob for now.
If it's hard to maintain it in the future, I like to use the glob for that.

> However there is a bug here. The htslib main package creates the
> libexecdir/htslib directory so needs to package the directory itself so that
> it will be deleted on uninstall. So what this really wants is
> 
> %files
> %{_libexecdir}/%{name}
> 
> or equivalent.

Thanks for checking!
I fixed the issue on following pull-request now.
https://src.fedoraproject.org/rpms/htslib/pull-request/2

I used %%dir for the directory. It looks safer as we can specify the files in the directory in %files.
I will apply the new build htslib-1.9-3 to master and fNN branches one by one.

Comment 26 Fedora Update System 2019-10-27 21:27:31 UTC
FEDORA-2019-c404cd1444 has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2019-c404cd1444

Comment 27 Fedora Update System 2019-10-27 21:29:05 UTC
FEDORA-2019-139769256b has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-139769256b

Comment 28 Fedora Update System 2019-10-27 21:30:04 UTC
FEDORA-2019-6b04023ef3 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-6b04023ef3

Comment 29 Fedora Update System 2019-11-05 03:56:53 UTC
htslib-1.9-3.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.

Comment 30 Fedora Update System 2019-11-05 04:15:08 UTC
htslib-1.9-3.fc31 has been pushed to the Fedora 31 stable repository. If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2019-11-05 05:28:44 UTC
htslib-1.9-3.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.