Implement PEP 518 and opt into PEP 517 builds #101
Conversation
pyproject.toml
Outdated
| @@ -0,0 +1,3 @@ | |||
| [build-system] | |||
| requires = ['setuptools ~= 41.4', 'wheel ~= 0.33.6'] | |||
There was a problem hiding this comment.
I do not think you want ~= here, it should be >=.
There was a problem hiding this comment.
I actually used the compatible release operator on purpose because the build is essentially an application. The nice thing about PEP 517/518 is that we get a dedicated environment to build in, so we don't have to worry about conflicts. There is no need to support multiple versions of build deps, and we don't want our application to break when a new incompatible release of a build dep is made.
There was a problem hiding this comment.
Again this is an unrelated change, which should be in its own PR if you feel it's justifiable. I'm not sure I agree with your premise that "the build is essentially an application", though, so I'm not sure that such a PR would be obviously something we'd want to add. That discussion can be had on the new PR, though, if you submit one.
There was a problem hiding this comment.
this is an unrelated change
🤔 Isn't specifying build deps kinda the whole point of pyproject.toml?
There was a problem hiding this comment.
Regardless of the semantics of what is and isn't related, it is not generally recommended to pin your build dependencies like this.
I can see the merits of doing it, but it needs to be a wider discussion. I think pick a valid minimum version (the current versions are fine) and use >= for both, then start a new issue about whether the sample repo should pin to an arbitrary version.
There was a problem hiding this comment.
Didn't see any defaults in PEP 517 (maybe you're thinking of pip's defaults? I believe those are meant for when build-system is omitted, though, which is not recommended AFAIK), so I copied from PEP 518 instead: https://www.python.org/dev/peps/pep-0518/#build-system-table
Not sure why Travis doesn't seem to like that 🤔
There was a problem hiding this comment.
used by people who don't have sufficient experience to judge whether pinning is the right choice.
Just curious, do you have a case in mind where it would be better to not pin?
There was a problem hiding this comment.
Just curious, do you have a case in mind where it would be better to not pin?
There is a difference of philosophies here that I think might be the problem, and it makes me think that maybe there is room for improvement in PEP 518 with regards to this issue.
From my perspective, the build-system.requires is similar to install_requires, in that it is an expression of the general requirements for the build system as you know them, which is to say that you should probably put the minimum version of setuptools or whatever that supports the feature set that you require.
You are using it more as a lock file akin to requirements.txt, which is to say that you are describing the actual versions to be used to do the build. This is usually an unnecessary restriction, though you are right that a breaking change in setuptools or wheel could cause old builds using PEP 518 to fail.
I think that the solution here would be to standardize on a lock file that can be included in an sdist (and possibly a git repository if you are paranoid) that describes the version of the packages used to build the relevant package.
In any case, there's currently no way to auto-update these files or warn on overly strict pinning here (plus if there were a way to auto-update them, what would be the source of truth?), so I can almost guarantee you that what a strict pinning in sampleproject will do is:
- Make it so a bunch of projects are unnecessarily pinned on this version of
setuptoolsforever, since people will copy this repo once and then never look at theirpyproject.tomlagain. This will be a real problem for the adoption of new setuptools features, particularly whenpipintroduces some change that reveals a bug insetuptools. - Make it so we need to set up a bot or something to automatically update the
pyproject.tomlevery timesetuptoolsorwheelcuts a release, causing a ton of unnecessary code churn.
I think we should do this in baby steps and use >= for the minimal setuptools and wheel versions that support the features we need (setup.cfg support, PEP 517 support) and then have a conversation on Discourse about the possibility of adding a build-time lock file.
There was a problem hiding this comment.
Didn't see any defaults in PEP 517
Sorry, yes I meant PEP 518. My bad. And the minimum version requirement on setuptools (which does come from pip) is simply to have a version that actually includes a PEP 517 backend at all.
There is a difference of philosophies here that I think might be the problem
Agreed. My view matches @pganssle's - the requires key should specify the minimum versions that you know will work, not pin exact versions. And my impression of the discussions we had when we were developing PEP 518 was that everyone was thinking in terms of minimum requirements, and very definitely not in terms of pinning. So yes, it is true that we don't really have a good way to pin exact versions at the moment.
I think we should do this in baby steps and use >= for the minimal setuptools and wheel versions that support the features we need (setup.cfg support, PEP 517 support) and then have a conversation on Discourse about the possibility of adding a build-time lock file.
+1 on this.
There was a problem hiding this comment.
Thank you both for iterating on this with me. I've switched to the pip defaults, and Travis seems cool with that now 😊
There is a difference of philosophies here
The difference here really seems to be more about whether to prioritize the latest deps or a reliable build...
The con of tighter pins is build deps potentially getting old. If this project doesn't have a need for newer versions, I don't really see that as a problem. I DO see breaking the build because of unrelated events as a problem. A build-time lock file sounds like an interesting and excellent way to deal with this... but it doesn't exist today. What we have today is essentially a requirements.txt and a build-time lock file would basically upgrade that to Pipfile+Pipfile.lock. So, the question is: what do we do with our requirements.txt: promote new versions or prevent the build from breaking.
general requirements for the build system as you know them
But, there is a correct answer for what goes in the build deps. It may not be worthwhile to track it down exactly (e.g. by figuring out exactly the versions that have support for all the features you need), but that doesn't mean that dropping in an approximation is just as correct.
Make it so a bunch of projects are unnecessarily pinned on this version of
setuptoolsforever
pipintroduces some change that reveals a bug insetuptools.
I kinda get this and kinda not... On one hand, so what? If it works for those projects, what's the problem. If it doesn't work (e.g. because of a new pip), then they will fix it, or they won't because the project is probably unsupported/abandoned anyways. After all, you will know when your build has broken.
On the other hand, I get it... maybe the best route is to explain all this in a comment accompanying the pip defaults. I've added that.
that support the features we need
Do we know if the pip defaults deliver on that?
the
requireskey should specify the minimum versions that you know will work, not pin exact versions.
In install_requires, you do this because downstream can work around new breaking releases by pre-installing. How do you work around broken build deps?
everyone was thinking in terms of minimum requirements
This is a good point and should go a long way, but at the end of the day... It's the technical behaviors that matter, IMO.
dmtucker
changed the title
dmtucker
force-pushed
the
master
branch
4 times, most recently
from
1212879
to
e0a0b3b
Compare
pyproject.toml
Outdated
| # These are Pip's default requirements: | ||
| # https://pip.pypa.io/en/stable/reference/pip/#pep-517-and-518-support | ||
| requires = ["setuptools>=40.8.0", "wheel"] | ||
| # Consider pinning YOUR build requirements more strictly. |
There was a problem hiding this comment.
You really should remove this. That is not something we want to encourage and it is not the semantic intention of requires, so in an exemplar project like this we really don't want to cause a bunch of people to get stuck on old versions of setuptools.
There was a problem hiding this comment.
How about this instead?
Beware that new backwards-incompatible releases may break the build.
There was a problem hiding this comment.
Please can you stop pushing this - both @pganssle and I have requested that we follow the "minimum requirements" approach, and leave pinning as something to discuss elsewhere. Please just remove the comment altogether.
There was a problem hiding this comment.
Thank you.
(BTW, are you force-pushing amended commits here? my comments seem to be getting detached from the code they were about).
There was a problem hiding this comment.
Sorry, you hadn't mentioned being against even warning the (probably new) user about the issue...
There was a problem hiding this comment.
are you force-pushing amended commits here?
Ya, just git commit -a --amend --no-edit && git push -f... I think maybe it's a GitHub thing depending on the particular line a comment was left. Not sure, but maybe doing multiline comments would help?
|
@dmtucker Thank you for this, you brought up some interesting points that we really need to figure out for PEP 518 builds in the future. |
Close #79