Bug 255722 - [meta] Mk/Uses/python.mk: PEP-517 (pyproject.toml) Support
Summary: [meta] Mk/Uses/python.mk: PEP-517 (pyproject.toml) Support
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Many People
Assignee: freebsd-python (Nobody)
URL: https://wiki.freebsd.org/Python/PEP-517
Keywords: feature, needs-patch, needs-qa
Depends on:
Blocks: 264679 265660 265692 265693 266681 268018
  Show dependency treegraph
 
Reported: 2021-05-09 08:14 UTC by Yuri Victorovich
Modified: 2022-11-27 21:29 UTC (History)
8 users (show)

See Also:
vishwin: maintainer-feedback+


Attachments
WIP.patch (2.66 KB, patch)
2022-08-05 17:38 UTC, Yuri Victorovich
no flags Details | Diff
WIP-2.patch (3.49 KB, patch)
2022-08-06 22:29 UTC, Yuri Victorovich
no flags Details | Diff
WIP-3.patch (2.64 KB, patch)
2022-08-07 17:47 UTC, Yuri Victorovich
no flags Details | Diff
RC-1.patch (2.42 KB, patch)
2022-08-07 18:23 UTC, Yuri Victorovich
no flags Details | Diff
RC-2.patch (2.93 KB, patch)
2022-08-07 19:41 UTC, Yuri Victorovich
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuri Victorovich freebsd_committer 2021-05-09 08:14:01 UTC
The source tarball for this project https://github.com/theislab/anndata doesn't have setup.py on the PyPI website.

distutils is being deprecated, see https://www.python.org/dev/peps/pep-0632/

The replacement, setuptools, uses pyproject.toml instead, but Mk/Uses/python.mk doesn't seem to support it. The relevant section of the handbooks doesn't say anything about this: https://docs.freebsd.org/en/books/porters-handbook/special/#using-python
Comment 1 Loïc Bartoletti freebsd_committer 2021-05-10 07:03:56 UTC
Hi Yuri,

We have already some ports using pyproject.toml (sip related IIRC): 

- https://cgit.freebsd.org/ports/tree/devel/py-qt5-qscintilla2/Makefile#n32
- https://cgit.freebsd.org/ports/tree/graphics/py-python-poppler-qt5/Makefile#n35

But I agree with you, a variable for pyproject.toml wouldn't hurt.
Comment 2 flying-sheep 2021-05-11 19:17:07 UTC
I created an account here to clear up some confusion:

pyproject.toml doesn’t have anything to do with setuptools *or* sip.

As of 6 years ago, it’s *the* standard way to specify how a python package is built, defined in https://www.python.org/dev/peps/pep-0517/#source-trees

This means there’s two ways do define python package builds: This standard one and the legacy setup.py

I described here how building an OS package from a python repo/sdist in a generic way could look: https://github.com/theislab/anndata/issues/561#issuecomment-835770254
Comment 3 Yuri Victorovich freebsd_committer 2021-05-11 19:56:13 UTC
(In reply to flying-sheep from comment #2)

flying-sheep@web.de,

Thank you for your comments.

Our python.mk needs to be updated to reflect the new standard.


Yuri
Comment 4 Yuri Victorovich freebsd_committer 2022-08-05 17:38:51 UTC
Created attachment 235704 [details]
WIP.patch

Add a very early draft of USE_PYTHON=build feature.
Comment 5 Yuri Victorovich freebsd_committer 2022-08-06 22:29:12 UTC
Created attachment 235732 [details]
WIP-2.patch
Comment 6 Yuri Victorovich freebsd_committer 2022-08-07 17:47:30 UTC
Created attachment 235745 [details]
WIP-3.patch
Comment 7 Yuri Victorovich freebsd_committer 2022-08-07 18:23:24 UTC
Created attachment 235750 [details]
RC-1.patch
Comment 8 Yuri Victorovich freebsd_committer 2022-08-07 19:41:24 UTC
Created attachment 235751 [details]
RC-2.patch
Comment 9 Charlie Li freebsd_committer 2022-08-07 21:54:26 UTC
Please see https://wiki.freebsd.org/Python/PEP-517 that I wrote up months ago but didn't have enough time to fully implement yet.

tl;dr it's way more involved than USES_PYTHON=build (and should not be that)
Comment 10 Yuri Victorovich freebsd_committer 2022-08-07 23:17:49 UTC
(In reply to Charlie Li from comment #9)

Hi Charlie,

Thanks for your comment.

Current implementation is functionally almost identical to your description in https://wiki.freebsd.org/Python/PEP-517:
* It is PEP-517 compliant.
* It requires port maintainers to parse pyproject.toml and add DEPENDS lines accordingly (this is also done in the supplied examples).
* It checks that requirements are met, and fails otherwise.

Differences:
* I used USE_PYTHON=build instead of USE_PYTHON=pep517: the user intent here is to build the project. IMO there's no need to put the standard name into Makefiles in many places. "build" is a lot easier to remember.
* autoplist isn't currently supported. It isn't obvious that it should be.

Benefits of this implementation:
* It is minimalistic. It does exactly what is required with as little code as possible.
* It uses only build/installer packages, a minimally required set of dependencies, and still provides the same functionality.


Best,
Yuri
Comment 11 Charlie Li freebsd_committer 2022-08-07 23:40:04 UTC
autoplist is supported, just a different way of doing it (and much less fragile).

"Minimalistic" will not work here, because a major part of PEP-517 is completely deprecating the setup.py or otherwise distutils path in package management and standardising on wheels. Even though setuptools includes an internal copy of distutils, there is no guarantee of it still existing in the future. Thus, in terms of future-proofing, this cannot go into the framework without also converting the toolchain to setuptools-less bootstrapping.

USE_PYTHON=build actually is misleading, because it's only one (minimalist) tool that only builds, not installs. In fact, py-build and py-installer didn't exist until after PEP-517 was accepted and (ensure)pip gained support.
Comment 12 Yuri Victorovich freebsd_committer 2022-08-07 23:51:48 UTC
(In reply to Charlie Li from comment #11)

Dear Charlie,

distutils and setuptools aren't required ot be present in the suggested approach. They would only be needed if pyproject.toml would explicitly specify them as a dependency.

"build" is a user intent, not a package name. It is such so that it can be easily remembered.

> [...] in terms of future-proofing, this cannot go into the framework without also converting the toolchain to setuptools-less bootstrapping.

This is a more global issue. setuptools and distutils aren't going anywhere regardless of what standards say, simply because there are so many projects that use them. This potential problem can be solved once it becomes a real problem. And it won't become a real problem any time soon.


Yuri
Comment 13 Charlie Li freebsd_committer 2022-08-08 00:08:17 UTC
> distutils and setuptools aren't required ot be present in the suggested approach

They are present by virtue of py-build and py-installer currently in the tree using distutils/setuptools to build themselves. installer is actually built wrong in this case since the specified build backend is flit, not setuptools (but works by coincidence).

> "build" is a user intent, not a package name

Still misleading, because the intent does not imply install to staging/wherever.

> This potential problem can be solved once it becomes a real problem. And it won't become a real problem any time soon.

It will become a real problem in our tree when PEP-517 support lands in the framework. Currently, when USES_PYTHON=distutils is specified, setuptools is an unconditional RUN_DEPENDS, which is unnecessary and wrong for PEP-517.
Comment 14 Yuri Victorovich freebsd_committer 2022-08-08 00:29:47 UTC
(In reply to Charlie Li from comment #13)

If you are implying that all instances of setuptools/distutils use would be removed form the ports tree for strict PEP-517 - this is a non-starter at the moment because of sheer number of non-compliant packages.

To be clear, the current patch introduces a local PEP-517 compliance so that the projects that have already switched to PEP-517 can be ported. This opens the path forward to a lot of software to enter the ports tree. Strict global PEP-517 compliance is something else that can be gradually addressed at a later time. This patch doesn't present an obstacle to this.


Yuri
Comment 15 Charlie Li freebsd_committer 2022-08-08 00:51:55 UTC
> If you are implying that all instances of setuptools/distutils use would be removed form the ports tree
Not what I meant, since setuptools is also itself a PEP-517 build backend.

The current patch is a hack at best, much like my phab reviews marked WIP. Functionality that USE_PYTHON=distutils provided that is available in PEP-517 (like autoplist) has to be provided. Additionally, the "toolchain" has to bootstrap itself through the framework. There is no gradual as far as the framework is concerned, but individual packages can go at their pace.

To further clarify, the best "testsuite" for the framework support is the (self-)bootstrap "toolchain" listed in the wiki.
Comment 16 Yuri Victorovich freebsd_committer 2022-08-08 01:00:58 UTC
(In reply to Charlie Li from comment #15)

Charlie,

You have probably misunderstood. The purpose of this patch is to allow individual packages to proceed at their own pace. Some of them currently can't enter the ports tree. This patch doesn't aim to enable global PEP-517 compliance, but it also doesn't stand in its way.

If you mean that the whole tree should be converted to PEP-517 at once - this is not practically possible. But this is an entirely orthogonal matter to the patch in question.

There is also no reason why this patch would prevent self-bootstrapping.


Yuri
Comment 17 Charlie Li freebsd_committer 2022-08-08 02:01:07 UTC
> If you mean that the whole tree should be converted to PEP-517 at once - this is not practically possible.
Also not what I meant. I agree that individual ports/packages should proceed at their own paces.

Bottom line, this really cannot be anywhere near rushed and has to be thought through holistically, even though there exist prospective ports that need the framework. The self-bootstrapping bit is merely an example of required flexibility in the framework (which does not exist in this WIP), in their case to eliminate circular dependencies. I updated the wiki page to clarify on some design goals that need met.
Comment 18 Yuri Victorovich freebsd_committer 2022-08-21 20:16:11 UTC
Committed.
Comment 19 commit-hook freebsd_committer 2022-08-21 20:17:04 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=600312a9118bc7b9c5bb7decbefe706f9046507b

commit 600312a9118bc7b9c5bb7decbefe706f9046507b
Author:     Yuri Victorovich <yuri@FreeBSD.org>
AuthorDate: 2022-08-21 19:46:39 +0000
Commit:     Yuri Victorovich <yuri@FreeBSD.org>
CommitDate: 2022-08-21 20:16:06 +0000

    Mk/Uses/python.mk: Add USE_PYTHON=build to support pyproject.toml based projects

    USE_PYTHON=build supports PEP-517 at the level of individual ports.
    Global support (making PEP-517 be used for all ports) is outside of the
    scope of this patch.

    PR:             255722
    Approved by:    python (maintainer's timeout; 14 days)
    Differential Revision:  https://reviews.freebsd.org/D36061

 Mk/Uses/python.mk | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)
Comment 20 Charlie Li freebsd_committer 2022-08-22 00:45:54 UTC
This was not approved or timed out. Please back out as soon as practicable.
Comment 21 Kubilay Kocak freebsd_committer freebsd_triage 2022-08-22 01:04:06 UTC
@Yuri Please revert as soon as possible. 

Proposed change received feedback with additional design notes (comment 17, among others) provided to which no feedback from you was received. 

Not withstanding, committing under maintainer timeout (13 days) after having received review and comment, by way of non-response, is not within the spirit of the maintainer timeout guidelines
Comment 22 Yuri Victorovich freebsd_committer 2022-08-22 01:18:53 UTC
(In reply to Kubilay Kocak from comment #21)

Kubilay,

> [...] committing under maintainer timeout (13 days) [...]

14 full days passed.


> [...] after having received review and comment [...]

I asked for specifics and there was nothing specific.
What should be done in in such case? Let it sit dormant for another 6 months?

python@ had a chance to disapprove the review and he didn't.



Yuri
Comment 23 commit-hook freebsd_committer 2022-08-22 01:44:07 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=7d48381cbd686ee8d77871bc25cf1e926a314f85

commit 7d48381cbd686ee8d77871bc25cf1e926a314f85
Author:     Charlie Li <vishwin@FreeBSD.org>
AuthorDate: 2022-08-22 01:39:08 +0000
Commit:     Charlie Li <vishwin@FreeBSD.org>
CommitDate: 2022-08-22 01:39:08 +0000

    Uses/python.mk: revert unapproved feature addition (and consumers)

    Change proposal was in discussion with open questions and additional
    documented design requirement [0] for submitter to review and provide
    feedback on, which was not provided.

    Submitter (and anyone else) is welcome to work with python@ to
    produce an appropriately reviewed feature.

    [0] https://wiki.freebsd.org/Python/PEP-517

    With hat: python (vishwin, koobs)
    PR: 255722, 265660, 265692, 265693
    Approved by: fluffy (mentor)

 Mk/Uses/python.mk                   |  48 +---------------
 devel/Makefile                      |   1 -
 devel/py-hatch-vcs/Makefile (gone)  |  23 --------
 devel/py-hatch-vcs/distinfo (gone)  |   3 -
 devel/py-hatch-vcs/pkg-descr (gone) |   4 --
 devel/py-hatch-vcs/pkg-plist (gone) |  20 -------
 misc/Makefile                       |   2 -
 misc/py-anndata/Makefile (gone)     |  30 ----------
 misc/py-anndata/distinfo (gone)     |   3 -
 misc/py-anndata/pkg-descr (gone)    |   4 --
 misc/py-anndata/pkg-plist (gone)    | 111 ------------------------------------
 misc/py-hist/Makefile (gone)        |  36 ------------
 misc/py-hist/distinfo (gone)        |   3 -
 misc/py-hist/pkg-descr (gone)       |   6 --
 misc/py-hist/pkg-plist (gone)       |  69 ----------------------
 15 files changed, 1 insertion(+), 362 deletions(-)
Comment 24 Yuri Victorovich freebsd_committer 2022-10-24 18:24:45 UTC
This is in my TODO list.
Comment 25 Kubilay Kocak freebsd_committer freebsd_triage 2022-11-16 22:16:33 UTC
^Triage: Level up importance and classify
Comment 26 Charlie Li freebsd_committer 2022-11-16 22:40:03 UTC
This is still in python@'s purview, and is still not timed out.

Certain bootstrap packages mentioned in the wiki page have finally converted to not using setuptools to build, so the "test suite" at the very least can be properly implemented and tested.

Additionally, build has not been the only minimalist PEP-517 builder for some time now (Gentoo's portage uses a different package(s) that still comply with the standard)
Comment 27 Charlie Li freebsd_committer 2022-11-16 23:08:53 UTC
Note: the framework is to at least account for overridable variables needed to bootstrap devel/py-flit-core in a self-hosting manner, ie what review D34786 does. This includes the autoplist procedure, which may be better implemented using a proper CSV parser, perhaps in Python itself using the built-in csv library.
Comment 28 Thierry Thomas freebsd_committer 2022-11-18 12:31:30 UTC
(In reply to Charlie Li from comment #27)

Do we really need autoplist?

It is not difficult for a porter to run `make makeplist', and it is much more interesting to have a fixed pkg-plist: you can really see what will be installed, and you can check the diff (missing files) when upgrading a port.
Comment 29 Charlie Li freebsd_committer 2022-11-18 14:49:48 UTC
autoplist is necessary, not only because it is supported with distutils, but RECORD is the wheel specification PEP-427's stronger (as in nearly every entry is hashed, something we don't do yet) equivalent of our plist. You can already really see what gets installed and compare versions by diffing RECORD (though the hashes will differ)
Comment 30 Thierry Thomas freebsd_committer 2022-11-18 15:24:43 UTC
I do not know where this RECORD is available, but I doubt that it will let you know which files will be installed by a port before its installation.
Comment 31 Charlie Li freebsd_committer 2022-11-18 16:21:16 UTC
RECORD is always in .dist-info, which is metadata unconditionally installed for every wheel. autoplist currently only pulls from that file.
Comment 32 Charlie Li freebsd_committer 2022-11-18 16:42:14 UTC
RECORD was initially defined in PEP-376; the canonical standard is now maintained as https://packaging.python.org/en/latest/specifications/recording-installed-packages/