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

Re: [metaphlan] Added autopkgtest and distutils patch — please review



Hi Harish,

harish chavre, on 2025-05-22:
>  I have updated d/changelog  and d/copyright as per  the feedback [0].

Thanks for your work on the autopkgtest and the integration of
test data in the auxiliary package, the latter work is a
relatively advanced packaging task.  I struggled a bit more than
I initially planned with polishing the integration of test data
in all gbp branches, hence part of the delay.  I brought a
couple of finishing touches to metaphlan and triggered an
upload.  Thank you for your contribution!

There is a leftover issue raised by the piuparts test[a], which
boils down to an issue in the dependency metaphlan2-data.  There
is a small chance that it could prevent migration of metaphlan
to testing, in which case it is good idea to keep an eye on the
metaphlan package tracker[b] to monitor how the situation
evolves in the upcoming days.

[a]: https://salsa.debian.org/med-team/metaphlan/-/jobs/7631493
[b]: https://tracker.debian.org/pkg/metaphlan

If you have a peek at the git log, you may notice I brought a
couple more changes than just around the autopkgtest and
copyright file.  This is because I checked for various quality
issues flagged by lintian.  I have shortcuts in place to
systematically run the following command against source and
binary packages after building them:

	$ lintian --display-info --pedantic --tag-display-limit=0

For future packages, you probably want to have a look at
outstanding issues raised by Lintian, although not all changes
may be welcome during the time of the freeze in which we are at
the moment.

I have further comments below.

> On Wednesday, 21 May, 2025 at 02:09:05 am IST, Étienne Mollier <emollier@debian.org> wrote:  
>> About the d/copyright file, the paragraph you added clarifies
>> the origin of the dataset.  Note however that in its current
>> form, the d/copyright file is malformed regarding the copyright
>> format v1.0 normalisation[2]:
>> 
>>  1. the "Files: debian-tests-data" stanza needs to appear after
>>     the header paragraph (which starts with the Format stanza),
>>     right now it appears before and this breaks the conformity
>>     to copyright v1.0, thus breaking automated license checking
>>     tools;

I implemented a header swap to restore conformity to copyright
format v1.0.  See 895336df12c991a27eeab6a9bccb870b22258e19.

>>  2. as documented in copyright v1.0, the Files stanza does not
>>     admit a Source field, but since the information you provided
>>     is essential to understanding where the data comes from, I
>>     think that you should use a Comment field instead (you can
>>     find an example how to use this field in other packages on
>>     Salsa[3]).

Thanks for adjustment eae87a24ac582ef7e35110bd5c5851d708e4706e.

>> Also, you accumulated several changes to the package, but the
>> file d/changelog does not reflect those changes yet.  A quick
>> and easy way to get a template to document your changes is to
>> use:
>> 
>>     $ gbp dch
>> 
>> This will read your git log and dynamically construct a template
>> entry for your Debian changelog.  Be wary that, since I tagged
>> the version including your component debian-tests-data as
>> version 4.0.4+ds, you need to restart the packaging version from
>> 4.0.4+ds-1, otherwise gbp dch will trip on the carpet by
>> proposing to set version 4.0.4-2 which would be incorrecte with
>> your component tarball injection.  When you finalize your
>> changelog entries, be notably wary to indicate that your new
>> autopkgtest "Closes: #1035175", so that the corresponding bug
>> entry[4] gets automatically closed on package upload to the
>> archive.  Also, please keep the distribution as UNRELEASED while
>> the package is not deemed fit for upload yet.  This allows any
>> team member to determine that the package includes changes but
>> is still work in progress.

I probably should have highlighted that while gbp dch works
great to generate a template of d/changelog entry, it also needs
a manual review.  The file is intended to document each change
brought to the package compared to it's previously uploaded
version.  I proceeded to a couple of adjustments to make the
changelog more readable and remove dupliaces.  Please look at
85c192c417bb69d1211a8ab6517d5c0a157f2b71, which also include
some further comments on changes I made to the package before
upload to account for various issues flagged by Lintian.  Note
that to facilitate the reading of the changelog, it is better to
associate the Closes statement to the changelog entry that
justified the closure of the bug #1035175.

> also will send pull request to upstream before that could you please review commit in my forked repository [1].

You proposed patch looks good to me as it matches what has been
discussed on Debian Med Matrix channel.  I believe that you can
propose the patch upstream.  If they are happy with your changes
too, then they may merge them.  Once you triggered the pull
request, you can add a Forwarded field to the patch header in
the metaphlan package, in order to document that the patch has
been forwarded upstream and may be removed in upcoming new
upstream versions.

> [0] https://salsa.debian.org/med-team/metaphlan/-/commits/master
> [1] https://github.com/biobakery/MetaPhlAn/compare/master...HarishChavre:MetaPhlAn:master

Have a nice day,  :)
-- 
  .''`.  Étienne Mollier <emollier@debian.org>
 : :' :  pgp: 8f91 b227 c7d6 f2b1 948c  8236 793c f67e 8f0d 11da
 `. `'   sent from /dev/pts/3, please excuse my verbosity
   `-    on air: Pattern-Seeking Animals - No Land's Man

Attachment: signature.asc
Description: PGP signature


Reply to: