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

Re: git-multimail 1.6.0 package review



On 2022-09-18 11:10:24, Bo YU wrote:
> Hi,
> On Thu, Sep 15, 2022 at 05:32:33PM +0100, Antoine Beaupré wrote:
>>Hi!
>>
>>I've done a quick review of the 1.6.0 package on salsa as of commit
>>d5bd184a1cf73b752f80dea46d8080493a5e663b.

[...]

>>Also, I didn't quite follow the work on the test cases, but why did you
>>replace pep8 by pycodestyle in the patch in debian/patches? The patch
>>itself doesn't actually explain the *why* (it explains the "what" but we
>>typically want more than this...)
>
> This time i add dep3 header for the patch. It should be noted that
> despite this patch, it is still not helpful for upstream test cases.
> I have forwarded this for upstream:
> https://github.com/git-multimail/git-multimail/issues/221#issuecomment-1245009306
> (To avoid bring noisy for upstream, i just recorded it in a issue)

I don't think pull requests are noisy... you should probably just submit
this as a PR upstream.

[...]

>>I'm also surprised we need that launcher at all. Normally, the
>>`setup.py` wrapper has a scripts= stanza which should install the
>>upstream one, why do we do it differently?
>
> yeah. The reason why I use launcher here is bacause that @jcfp helped
> me to review it in the previous time:
>
> ```
> the (large) git_multimail.py file is installed twice, both as a
> public module under /usr/lib/python3 and as a script in /usr/bin;
> the latter could be replaced by a tiny launcher (take a look at the
> nfoview package if you need inspiration; your launcher would be even
> shorter because it doesn't need the sys.path manipulation)
> ```
> I am not sure if I understand jcfp's meaning correctly and I refer to
> nfoview:
> https://salsa.debian.org/python-team/packages/nfoview/-/blob/debian/master/debian/launcher/nfoview
>
> The issue is that I installed git_multimail.py twice in fact it should
> be under /usr/lib/python3 only as jcfp said. So i remove it in /usr/bin
> by hand.
>
> Now I have removed the launcher for git-multimail and it should work:)

Hm. This is weird. Why would you *not* want git-multimail under
/usr/bin? I understand the that git_multimail.py *module* should be in
/usr/lib/, but you should *also* have a launcher in /usr/bin/

I think, therefore, this is incorrect:

+override_dh_python3:
+       dh_python3
+       rm -f debian/git-multimail/usr/bin/git_multimail.py
+

First off, don't use `-f` there: we *do* want to fail if the file
doesn't exist, so that we can remove the override.

Second, this looks wrong: `git-multimail` (the launcher) should be the
thing that's installed under /usr/bin, not `git_multimail.py` (the
module). If the module ends up in /usr/bin, then it's probably a bug
upstream that should be filed.

Could you clarify what happens with the package right now?

[...]

>>Finally, one fundamental issue with this package is that, as you
>>correctly identified, it's unmaintained upstream. If we do ship it in
>>Debian, maybe we'd want to keep it out of stable until that is settled?
>
> Ok. I think so also. Fortunately, maintainer of upstream has a little
> response, but that's all.

Okay, so the right way to do this is to file a release-critical bug
against the package once it enters Debian.

>>I think that's what I have for now. I haven't double-checked the
>>upstream branch to see if it matches the upstream repo I have here, but
>>that would be my next step before uploading... just a formality to make
>>sure everything matches up.
>>
>>Thanks for working on this package!
>
> Thanks. This is the first package made by me with you all help.
>
> The new version for git-multimail is here:
> https://mentors.debian.net/package/git-multimail/
>
> Thanks again for your encouragement.:)

I hope that helps! :)

a.

-- 
Omnis enim ex infirmitate feritas est.
All cruelty springs from weakness.
                         - Lucius Annaeus Seneca (58 AD)


Reply to: