[PATCHES] Improve Python package quality

Hi,

as announced in

https://lists.gnu.org/archive/html/guix-devel/2021-01/msg00021.htmlI’ve

been working on adding an additional phase to Python packages to check

whether they actually work. I cleaned up my patch, added tests and now

I’m pretty confident it works as expected. The first patch in this

series adds this phase, while the other ones fix build failures caused

by it. All of this should go to core-updates (or a separate wip-*

branch?), since it causes a massive number of rebuilds.

You can also pull my git repo at https://github.com/PromyLOPh/guix.git

branch work-python-importcheck.

Cheers,

Lars

Hi Lars,

thanks for the patch set. Please please find some comments. (I did not

check all changes to the packages, assuming you did it right :-)

Please but this int a new line - makeing it easier to copy & paste.

(Leading emptry lines doe nor effect "from __future__ import").

This looks very uncommon (and make my mind hooking on it). Please use

this, which is more common and less mindbogling ;-)

print('...checking requirements: ', end='')

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:

req = dist.as_requirement()

# dist.activate() is not enough to actually check requirements,

# we have to .require() it.

pkg_resources.require(str(req))

if group not in {'console_scripts', 'gui_scripts'}:

Why not gui-scripts?

If not adding gui-scripts, just use "if group != 'concolse_scrips':"

Here it is fine ;-)

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.

Please add a short explanation why there can be unavailable top-level

modules.

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?

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")

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:

setup(name='~a', packages=~a, install_requires=~a)

Arguments are not used?

Any reason for relaxing this? Why not use python-pytest-6 as input?

Please rephrase into something like

Do not validate loadability

Do not validate whetehr packag is loadable

…

Dosn't this change fix the checks? So this comment would be obsoltes and

"#:tests #t" can be removed.

I suggest keeping the old way, as this is will automatically update to

upstream changes.

Hartmut Goebel <hartmut@goebel-consult.de> writes:

You can also escape the line break:

--

Ricardo

Hi Hartmut,

adopted Ricardo’s suggestion here.

Done.

Yes, the string is used in the error message.

I wasn’t aware this exists. Added, thanks!

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.

Done.

I’m also not happy with the name, but can’t think of a better one right

now. Anyone?

Done.

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

Fixed.

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).

Done.

Should’ve been #f, sorry, fixed.

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

severity 45712 important

quit

Hi!

Lars-Dominik Braun <lars@6xq.net> writes:

Thanks for the initiative! It looks good, on first sight. One question

I have is this: does it rely on the Python package having been built

with setuptools/distutils? The Python world is moving toward a

plurality of PEP 517 compliant build systems; any idea if the checker

will continue working for these new packages?

We have currently only one such package in our collection, on

core-updates. It's python-isort, added with commit

812a2931de553d12c01b0a4d53d03613b09adaaf on the core-updates branch.

Thank you,

Maxim

Hi again,

[...]

I hadn't seen this patch but I fixed that problem on core-updates with

be7061cea30b59676fc473d6bd4a56a0f2fbd7cf on the 14 of January. Note

that python-pytest-6 became just 'python-pytest' there.

Hi Lars-Dominik,

Lars-Dominik Braun <lars@6xq.net> writes:

Attached is a small rework of your original patch. I've made the Python

script standalone, which should make it easier to maintain. I've also

refactored the tests somewhat and added your copyright information.

Is this OK with you?

Thanks!

Maxim

Hi Maxim,

yes and no. pkg_resources is part of setuptools, so my patch depends on

setuptools. But I think dependencies are specified in a standardized

format[1], which any tool can (and should?) write.

top_level.txt seems to be part of python eggs, which are deprecated[2].

I guess we could just scan for top-level directories under

site-packages, which have a __init__.py and try to load them.

I can’t find any standard for the entry points, so I’m guessing they’re

setuptools-specific too.

My assumption is that if no metadata is found the checks are just

skipped and nothing bad happens, but we may have to verify this.

As for PEP 517, I’m working on updating python-build-system, see

I’ll send an updated patch to guix-patches@ as soon as I have the 'check

phase working.

Cheers,

Lars

[1] https://packaging.python.org/specifications/core-metadata/#requires-dist-multiple-use

[2] https://setuptools.readthedocs.io/en/latest/deprecated/python_eggs.html#top-level-txt-conflict-management-metadata

Hi Maxim,

sure, no problem with the refactoring in gerenal, but I think you used

an old patchset. I sent a v2 to this issue, which had some changes to

the scripts and tests.

I can’t test your patch properly unfortunately, because `make check`

does not work on core-updates and trying to build any Python package on

core-updates triggers the requirements checker immediately:

validating 'attrs' /gnu/store/hdjip92izsf9anfhd6ijgc9glvbi4dzv-python-attrs-bootstrap-19.3.0/lib/python3.9/site-packages

...checking requirements: ERROR: attrs==19.3.0 The 'attrs==19.3.0' distribution was not found and is required by the application”)

Maybe that’s an issue with Python 3.9?

Cheers,

Lars

Hi Lars,

Lars-Dominik Braun <lars@6xq.net> writes:

Doesn't that mean that the attrs requirement should be lifted from the

bootstrap version? As its purpose is exactly this: breaking the cyclic

dependency with itself.

Maxim

Hi Maxim,

ah, now I see the problem, there’s a (add-installed-pythonpath inputs

outputs) missing and so it can’t find the installed packages. Maybe

we’re working on different branches? I applied your patch directly to

core-updates.

Cheers,

Lars

Lars-Dominik Braun <lars@6xq.net> writes:

[...]

I'm not sure what the above was caused by, but its'

been fixed on core-updates (the version of pytest as per its metadata is

reported correctly there).

Hi Lars,

Lars-Dominik Braun <lars@6xq.net> writes:

Oh yes, sorry I had failed to mention it, this is on the

cu/farewell-to-pythonpath branch, which is the integration of

GUIX_PYTHONPATH and this work of yours. I've included your v2 patches,

with light editing: fixed the script indentation (spot via flake8), I

removed the python2 tests as Python 2 is obsolete, and removed the space

between the (procedure)[area/conditional] part of the GNU changelog

commit messages.

Another note to help with review; when sending v2 patches, make sure the

title of your mail reply mentions [PATCH v2]; this helps to spot the

later versions in email threads.

The branch is shaping up nicely; I encourage you to try it. If no major

problem is found with it, I'll merge it in core-updates soon.

Thank you,

Maxim

Hi Maxim,

thanks!

Sorry about that, will do next time.

I’ve been rebuilding packages on my list that use python-build-system

using this branch and quite a few fail, but in a random sample I found

none that fails due to this patch and also I imagine some issues are

already fixed on core-updates? Is there anything else I can do?

Cheers,

Lars

Hi Lars,

Lars-Dominik Braun <lars@6xq.net> writes:

I guess we can be bold and merge the branch to core-updates, and fix any

breakage that may occur there!

I've done so, the last commit of the series is

1b9186828867e77af1f2ee6741063424f8256398.

Let's make Python on Guix great again!

Thank you :-)

Closing,

Maxim

Hi Maxim,

I had discussed some change to his original patch with Lars. Can't

remember all point, just these:

+ if group not in {'console_scripts', }:

"gui_scripts"m are missing. Using a set there is uncommon, since it is a

constant value anyway.

+ # And finally try to load top level modules. This should not have any

+ # side-effects.

I'd try loading the top level module first. As this is a pre-condition

for loading the entry-points.

--

+++hartmut

| Hartmut Goebel | |

| hartmut@goebel-consult.de | www.goebel-consult.de |

Hi Hartmut,

these points were addressed, before Maxim merged the change into

core-updates.

Cheers,

Lars