Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use PEP-517 build process for Pipenv, others #11754

Closed
wants to merge 3 commits into from

Conversation

danchr
Copy link
Member

@danchr danchr commented Jul 31, 2021

Description

This is related to https://trac.macports.org/ticket/63320 — we should probably use that for more ports, so I tried to enable it for a few of the Python ports I maintain.

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS x.y
Xcode x.y

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

@macportsbot
Copy link

Notifying maintainers:
@lbschenkel for port py-dulwich.

@macportsbot macportsbot added by: member Created by a member with commit rights maintainer: open Affects an openmaintainer port type: enhancement type: submission labels Jul 31, 2021
@danchr
Copy link
Member Author

danchr commented Jul 31, 2021

Oh, forgot that I wasn't the maintainer of Dulwich, sorry!

@lbschenkel
Copy link
Member

It's OK, you can go ahead.

@reneeotten
Copy link
Contributor

we'll probably need a change like this at some point, but before making changes to the python PG it should be tested properly on more than a few ports to make sure this will work correctly. This is the suggestion that was made by upstream when I asked about py-platformdirs but we have to see whether this works well for MacPorts.

Before potentially merging it we should at least have @jmroot weigh in on the suggested changes.

@reneeotten
Copy link
Contributor

@danchr the logs also show ERROR No module named 'importlib_resources' so you're likely missing a dependency somewhere; please check the logs for possible additional issues.

port:py${python.version}-pep517 \
port:py${python.version}-toml

if {${python.branch} < 3.8} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the typical way here would be if {${python.version} < 38} {. I am not sure if this should work, but there is the error with importlib_resources - is that needed here as well (didn't check myself yet...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a bit of digging, I think it was actually py-python-install that was missing a dependency. I've fixed it to use the usual style, thanks.

@@ -295,12 +295,13 @@ proc python_set_pep517 {option action args} {
return
}
global python.pep517 python.version subport name
if {$subport ne $name} {
if {[info exists python.version]} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without that change, you cannot use PEP-517 builds in single-version ports using python.version directly, such as pipenv.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look at refactoring this. It would probably work better as a callback anyway.

@danchr danchr force-pushed the pep517 branch 2 times, most recently from 34858a2 to 29ab540 Compare July 31, 2021 18:23
@danchr danchr requested a review from jmroot July 31, 2021 18:23
@danchr
Copy link
Member Author

danchr commented Jul 31, 2021

we'll probably need a change like this at some point, but before making changes to the python PG it should be tested properly on more than a few ports to make sure this will work correctly. This is the suggestion that was made by upstream when I asked about py-platformdirs but we have to see whether this works well for MacPorts.

Not many ports use the option, so I've added a commit to rev-bump them all.

@jmroot
Copy link
Member

jmroot commented Aug 1, 2021

OK, I'll first clear up a few points brought up here and in the originating ticket.

  • I am well aware of the evolving python packaging situation.
  • The assertion that setup.py install has been obsolete for 5+ years is unfounded. That is when the PEPs were written, but the supporting software, and just as importantly, support in individual projects, didn't appear until much more recently. platformdirs, which started this discussion, didn't have a pyproject.toml until May this year. Many less maintained projects probably still don't.
  • pip is unsuitable for distro packaging generally, because most of what it does either isn't wanted in that context or is redundant and potentially conflicting with what the distro's own tools are doing.
  • We are using pep517.build despite its deprecation for similar reasons. Importantly, it has less dependencies than build. It seems like pep517.build will eventually be split into its own package rather than just being deleted.
  • setup.py install works fine in the vast majority of cases.
  • Until a pep517 front end (and preferably also a minimal wheel installer, see above point regarding pip) is included in the standard library, setup.py install can't go away without creating severe bootstrap problems. It seems likely that that will come to pass given that distutils is deprecated in python 3.10 and there is currently no other way of building the stdlib modules. But for now, there isn't even a TOML parser in the stdlib (partly because the TOML 1.0 spec was only finalised in January this year.)
  • Because of the aforementioned bootstrap problems that are solved by setup.py install, no ports that are used as dependencies with python.pep517 can themselves install with that option. The maintainers of those dependencies have to be vigilant to keep things working if new dependencies are added in new versions.

@danchr
Copy link
Member Author

danchr commented Aug 1, 2021

OK, I'll first clear up a few points brought up here and in the originating ticket.

Thank you for elaborating — those are very valid reasons for not mucking too much about with the PortGroup 🙂 The build also failed with an obscure error in one of the ports, so I've removed the parts of the PR that touched the group. I assume it's still somewhat desirable to use PEP-517 for ports?

@jmroot
Copy link
Member

jmroot commented Aug 1, 2021

Yes, if maintainers want to switch their python module ports to use pep517, that's fine except in the case of the dependencies needed for that build style. That is a direction we should try to be moving in generally. But things do break sometimes, and each port needs to be tested.

@jmroot
Copy link
Member

jmroot commented Aug 1, 2021

And TBH I would just wait for a version update to do the switch to pep517, rather than rev bumping just for that. But that's up to the maintainers.

I will also look into whether using build would gain us significant desirable functionality, and whether that's worth the extra dependencies, and whether the dependencies can be reduced.

jmroot added a commit that referenced this pull request Aug 1, 2021
breun pushed a commit to breun/macports-ports that referenced this pull request Aug 3, 2021
@jmroot
Copy link
Member

jmroot commented Nov 2, 2021

I've updated the portgroup to use build. The main advantage is that it can check for dependencies and error when they are missing.

The changes to the portgroup and py-python-install in this PR are no longer needed. Enabling pep517 in the other ports should be fine.

@reneeotten
Copy link
Contributor

thanks @jmroot, let's just close this PR then...?

@reneeotten reneeotten closed this Nov 16, 2021
@danchr danchr deleted the pep517 branch January 17, 2022 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
by: member Created by a member with commit rights maintainer: open Affects an openmaintainer port type: enhancement type: submission
5 participants