[bug#45712] [PATCHES] Improve Python package quality

guix-patches

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

[bug#45712] [PATCHES] Improve Python package quality

From:	Lars-Dominik Braun
Subject:	[bug#45712] [PATCHES] Improve Python package quality
Date:	Tue, 12 Jan 2021 10:37:07 +0100

Hi Hartmut,

> Please but this int a new line - makeing it easier to copy & paste.
> (Leading emptry lines doe nor effect "from __future__ import").
adopted Ricardo’s suggestion here.

> This looks very uncommon (and make my mind hooking on it). Please use
> this, which is more common and less mindbogling ;-)
Done.

> Any reason for converting the req into a string first? IC,
> "`requirements` must be a string". So I'd move the "str()" to the
> function call:
Yes, the string is used in the error message.

>     if group not in {'console_scripts', 'gui_scripts'}:
> Why not gui-scripts?
> If not adding gui-scripts, just use "if group != 'concolse_scrips':"
I wasn’t aware this exists. Added, thanks!

> Does it make sence to try loading the top-level modules *after* the the
> entry-point? Chances are very high that the entry-points implicitly test
> the top-level modules already.
> IMHO it would make more sense to first try the top-level modules, and
> even stop processing if this fails.
True, I reversed the order. Still, I think continuing with the entry
points might be worth, if it points out more (different) errors. For
small packages this might not be the case, but larger ones often only
re-export specific names in the top-level module, thus testing might
reveal more issues.

> Please add a short explanation why there can be unavailable top-level
> modules.
Done.

> While not having antoher idea, I'm not happy with this name. "loadable"
> is not easy to get. (See also below, where this term is used in commit 
> message.)
> top-level-modules-are-importable?
I’m also not happy with the name, but can’t think of a better one right
now. Anyone?

> AFAIKS the packages only differ by some requirements. So I suggest
> using a function to return the packages. This makes it easier to spot
> the actull differences.
> […]
> (mkdir-p "src/dummy")
> (chdir "src")
Done.

> I would strip this down (version is not required AFAIK) and put it one
> line (her assuming you use (format) for creating the code within a function:
Not sure whether it’s optional or not, [1] does not say it is. No harm
in keeping it?

[1] 
https://setuptools.readthedocs.io/en/latest/references/keywords.html?highlight=version#keywords

> Arguments are not used?
Fixed.

> Any reason for relaxing this? Why not use python-pytest-6 as input?
Yes, our pytest@6 package reports its version as 0.0.0, so this patch is
required with either package. Or we figure out how to fix pytest@6
(works fine with PEP 517 compliant build system).

> Please rephrase into something like
> Do not validate loadability
Done.

> Dosn't this change fix the checks? So this comment would be obsoltes and
> "#:tests #t" can be removed.
Should’ve been #f, sorry, fixed.

> I suggest keeping the old way, as this is will automatically update to
> upstream changes.
Okay, I’ve added another phase that disables the test manually, because
it fails for me every time.

See attached patchset (v2) or git repository for updated patches.

I’ve also rebuilt all 2284 packages depending on python-build-system.
Currently 426 of them fail to build for any reason (not necessarily
caused by this patchset; I’ve seen some unrelated issues). Due to the
large number of packages failing I suggest moving forward with applying
this first batch and then fixing everything else incrementally, unless
some major package is broken (see attached list). WDYT?

Cheers,
Lars