[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Request for sponsoring changes in idba



Hi Pranav,

thanks to your work on the idba package.  At first a note for your environmet
setup.  You obviously used dch to create the changelog entry.  This added an
invalid e-mail address as changelog owner.  I recommend you set

   DEBEMAIL=ballaneypranav@gmail.com
   DEBFULLNAME='Pranav Ballaney'

(or whatever e-mail address you prefer in your Debian work) in your
environment to avoid this.  Using an Ubuntu system for your work is fine
but please note that dch adds a <Debrelease>ubuntu1 to the version instead
of just bumping the <Debrelease> number.  I can easily edit this before
uploading but just giving you a hint about this here.

On Sun, Feb 23, 2020 at 03:07:59PM +0530, Pranav Ballaney wrote:
> Hi,
> I have added autopkgtests for idba <https://salsa.debian.org/med-team/idba>.
> (Closes #909716 <https://bugs.debian.org/909716>)
> 
> I need a few suggestions on how these tests can be improved.
> 
>    1. The test data can be generated using the sim_reads command that comes
>    with the package, but for some reason, it isn't installed when the package
>    is built.

Ahhh, its always good if we can implement a 4 eyes principle to our
packages.  Just to introduce myself:  I'm not a bioinformatician but a
physicist who simply is able to create Debian packages and I'm not
*using* these packages and barely know what they can be used for.  I was
packaging idba according to user request and I was simply installing
those executables only that were requested by the user who asked me for.

I'm now realising that idba build system builds a lot of other tools
inside $(CURDIR)/bin which might (or might not?) be sensible to install.

>    I am being able to use it if I build the package from source on
>    my machine, but within the testing chroot, autopkgtest reports that this
>    command is not found.

(UNTESTED!) Please have a look at commit
133e974096b40aedc7108258b286f74296894f3a where I've set PATH to make sure
all binaries will be found.

>    If this command can be used while testing, the
>    downloads required for testing will be reduced from the current 25MB to a
>    mere 1.5MB, because the rest of the data can be generated as required.

I have not verified the test suite but it is not possible to download
anything while building the package since every package needs to be able
to build on a non-networked machine.  So every download has to be
patched from the test suite anyway.

>    For
>    now, I have generated and added all the files manually, and included the
>    code used to generate this data in a README file inside
>    debian/tests/test_data, if it becomes possible to run these while testing
>    at a later stage.

Its way more sensible to provide all needed tools inside the package.
so please decide which of **all** the tools in $(CURDIR)/bin should end
up in the binary package.  You can install these easily by for instance
adding a line like

   bin/sim_reads	usr/bin

to the file debian/install.

>    2. To copy the test_data to a temp folder inside the testing chroot, I
>    have added an install file inside the debian folder. Is this the right way
>    to do it?

In principle yes.  However, if we really need to keep any large static
data files (instead of generating these) I would prefere to add an additional
Architecture: all package (for instance named idba-data).  The rationale is
that Architecture: any files are duplicated for every Debian architecture
which is not bloating the Debian ftpmirror network with duplicated files but
also creates a lot of network traffic.  That's why we split up those binar
independent files into a separate binary package.  Moreover if the data are
not used to actually run the package we would also bloat user machines who
might just want to run idba in a cluster or so.
 
In short: Please check prefere creation of the files in the test over static
data.  Having a few small files provided statically is fine, thought.

Additional hint: Using the install file is fine, but its more elegant to
use either the file 

   debian/docs

containing

   test_data

or install the data in examples (either test_data subdir or directly by using

   debian/examples

containing
     test_data/*  (direct install)
  or
     test_data    (in subdir)

> I have also updated the changelog (and marked the bug resolved) and added a
> README.test file inside debian.

Very good!

> Please let me know if I missed out anything, and if not, please review and
> sponsor these changes.

Provided that your time permits

   1. Decide about what executables in $(CURDIR)/bin should be installed
   2. Generate as much data as possible
   3. Check whether test suite tries to download anything and ignore
      these tests or provide these data statically
   4. If we need large static data files create an extra binary package

Feel free to ask me about any of these steps if you might need any help!

Hope this helps

      Andreas.


-- 
http://fam-tille.de


Reply to: