Fixed #30948 -- Changed packaging to use setuptools declarative config in setup.cfg. #12013
Conversation
|
I was considering also removing the Python version check at the top of Same thought for the the overlay warning. Is this warning & logic still relevant with modern pip and setuptools? |
We should always be recommending If we decide to remove the overlay warning, I could also ensure all docs recommend |
There was a problem hiding this comment.
Thanks for taking this on Jon. I've been meaning to get around to it.
Could you also add a pyproject.toml with the following:
[build-system]
requires = ['setuptools>=30.3.0', 'wheel>=0.32.0']You may want to check whether we need newer versions than those I've specified - I just went with the minimum for setuptools for support of metadata in setup.cfg.
+1 to getting rid of the custom version check as python_requires has been around long enough now and this was originally introduced to deal with Python 2 support being dropped in Django 1.11 and python_requires being a bit too new. When 3.1 is released, Python 2 will have been EOL for 8 months. I also think this is sort of misleading now as the required Python version has been been bumped, but the instructions lead toward installing Django < 2.0, which isn't necessarily correct.
+1 to getting rid of the overlay warning. Using python setup.py install has been deprecated for a long time and use of pip install . or pip install --editable . is recommended.
P.S. There is also a spelling mistake in the commit message: declaritive → declarative.
setup.cfg
Outdated
| author_email = foundation@djangoproject.com | ||
| description = A high-level Python Web framework that encourages rapid development and clean, pragmatic design. | ||
| long_description = file: README.rst | ||
| license = BSD |
There was a problem hiding this comment.
I know that this was taken over from what is currently there in setup.py, I think we should be more clear.
Strictly speaking, Python is licensed under BSD 3-Clause "New" or "Revised" License which isn't clear from this and is not possible to specify in the classifiers. We should either use the full name or, better yet, the SPDX identifier: BSD-3-Clause. [1]
In addition, Django contains a copy of Python License 2.0 which was added as in the past some code was copied from the Python Standard Library. Strictly speaking that means that Django is (or should be) distributed under the terms of both licenses. That is also not clear here. Looking back through the history of LICENSE.python it was originally added for a backport of weakref which has since been removed. I thought that we could potentially remove it from master again, but grepping for the license filename, we have custom version of urllib.parse.parse_qsl() as django.utils.http.limited_parse_qsl(). [2]
So it seems to me that perhaps we ought to make this license = (BSD-3-Clause AND Python-2.0) and add License :: OSI Approved :: Python Software Foundation License to classifiers.
@carltongibson @felixxm I think this needs some looking into what the correct course of action is.
[1] Note that there is an ongoing discussion on how to make this better in the (probably now, not too far) future, but I think we can do better in the short term.
[2] I see that Python 3.8 has a max_num_fields argument that mirrors the fields_limit argument in the custom version - am prepping a PR to handle updating to a backport from 3.8 instead of the current implementation so that it can eventually be removed.
There was a problem hiding this comment.
PR to handle updating to a backport from 3.8
See #12017.
There was a problem hiding this comment.
I'll happily adjust the license lines to the most correct and explicit. I'll await instruction from @carltongibson @felixxm on how to proceed.
jdufresne
changed the title
Can you link or provide information as to what this will solve? In other words, when will this influence an action by a packager? I've been testing using All remaining feedback from the review has been incorporated. Thanks! |
I think that we also need to update the following: diff --git a/INSTALL b/INSTALL
index eb9cf6eaf2..c24c4bb028 100644
--- a/INSTALL
+++ b/INSTALL
@@ -1,10 +1,9 @@
Thanks for downloading Django.
To install it, make sure you have Python 3.6 or greater installed. Then run
-this command from the command prompt:
+the following commands from the command prompt:
- python setup.py install
-
-If you're upgrading from a previous version, you need to remove it first.
+ python -m ensurepip
+ python -m pip install .
For more detailed instructions, see docs/intro/install.txt.
diff --git a/docs/faq/troubleshooting.txt b/docs/faq/troubleshooting.txt
index ba44aa83ef..f90d0e8e6e 100644
--- a/docs/faq/troubleshooting.txt
+++ b/docs/faq/troubleshooting.txt
@@ -14,9 +14,9 @@ Problems running ``django-admin``
-----------------------------------
:doc:`django-admin </ref/django-admin>` should be on your system path if you
-installed Django via ``python setup.py``. If it's not on your path, you can
-find it in ``site-packages/django/bin``, where ``site-packages`` is a directory
-within your Python installation. Consider symlinking to :doc:`django-admin
+installed Django via ``pip``. If it's not on your path, you can find it in
+``site-packages/django/bin``, where ``site-packages`` is a directory within
+your Python installation. Consider symlinking to :doc:`django-admin
</ref/django-admin>` from some place on your path, such as
:file:`/usr/local/bin`.
diff --git a/docs/topics/auth/passwords.txt b/docs/topics/auth/passwords.txt
index 44e80911ba..134ef14583 100644
--- a/docs/topics/auth/passwords.txt
+++ b/docs/topics/auth/passwords.txt
@@ -89,7 +89,7 @@ To use Argon2 as your default storage algorithm, do the following:
#. Install the `argon2-cffi library`_. This can be done by running
``python -m pip install django[argon2]``, which is equivalent to
``python -m pip install argon2-cffi`` (along with any version requirement
- from Django's ``setup.py``).
+ from Django's ``setup.cfg``).
#. Modify :setting:`PASSWORD_HASHERS` to list ``Argon2PasswordHasher`` first.
That is, in your settings file, you'd put::
@@ -119,7 +119,7 @@ To use Bcrypt as your default storage algorithm, do the following:
#. Install the `bcrypt library`_. This can be done by running
``python -m pip install django[bcrypt]``, which is equivalent to
``python -m pip install bcrypt`` (along with any version requirement from
- Django's ``setup.py``).
+ Django's ``setup.cfg``).
#. Modify :setting:`PASSWORD_HASHERS` to list ``BCryptSHA256PasswordHasher``
first. That is, in your settings file, you'd put::We should probably also update |
You can find out more at the following links: |
|
Trying to specify old setuptools results in: So I'll use this as the minimum setuptools unless someone has a reason for a different version. |
|
Thanks for all the helpful feedback! Changes:
|
There was a problem hiding this comment.
Hi @jdufresne @pope1ni.
Thanks for this. A few things going on:
- The headline move to setup.cfg
- Adding pyproject.toml as well.
- Removing the Python 2.7 warning.
- Removing the overlay warning.
- Clarifying the licence. Comment
Maybe 1 & 2 can be addressed together, but I'd prefer to break things up into smaller changes, to be assessed individually.
On 5, first of all gulp. :) — I need to look into this. I'm going to ask @ubernostrum if it's something the board might be able to advise on?
On 3, would you think me particularly old-fashioned if I said that I'd prefer to hold-off on dropping this until well after the Python 2.7 EOL, e.g. until the end of 2020 say? With modern versions of... — yes, but I take it we're catching the case of someone who just happens to have some Python, they know not what installed and downloads Django to find an inconsistency. It costs us nothing to have this here for a season longer.
On 4, not sure what to say immediately, but (as per above) I'd like to address it separately if that's OK.
On 1 and 2, I'm inclined towards being super conservative here. (For all manner of reasons, the main one being I don't want to break anything.) But it looks OK at first glance, so trimmed down, and with a bit of time to play with, no doubt you'll persuade us.
Super effort. Thank you.
Sure.
Heh. Yeah. I figured that this was an oversight w.r.t. to the dual licensing bit. But would be good to clarify. It just doesn't quite feel right...
I figure that there isn't much difference between August 2020 - when Django 3.1 will be release with these changes - and December 2020... If we decide to keep it until Django 3.2, let's put a comment in to that effect so that we can remember to scrub it when development on 3.2 starts.
Yeah. So I think the main issue here is due to installing on top of an existing install in the global site packages using
I don't believe we've changed anything of substance? Do correct me if I'm wrong... 😛
A pleasure! |
It doesn't look that way, but let's trim it down and then we'll see. |
Why don't we punt on this action item then? This PR keeps the license exactly as it was before, no change. If there is a decision made that the license needs updating, that can easily happen after this lands (or even before, really). Whether or not we're using |
|
Yep, that's fine. (It's something to handle apart from this PR.) (Super happy to look at it but...) |
Yup, I can add that back, but some context: Here is my experience installing Django with pip on Python 2. Notice it chooses Django 1.11, not Django 2.0+. Without this version checking, here is my experience installing Django 3.1 from the source directory using Python 2 With the error messaging reinstated: Installing without the check certainly has less pleasant error message, but it will not allow the user to proceed with an incompatible version. From your perspective, is this all about the user messaging?
I agree with this sentiment, but am happy to comply. I can add this comment if it is agreeable with @carltongibson |
|
I have restored the version check and overlay check in setup.py. I have also included a TODO comment to remove this at a future date. Thanks for all the feedback! |
|
@jdufresne This check is not only for Python 2, you can try to install e.g. Django 3.0 with Python 3.5 via |
|
Here is setuptools purging it from their docs: I think we should do the same. It is only referenced in internals docs: |
Let's just consider at least changing |
|
Taking a stripped down version of what Jon posted:
So this is Python 2 using
I would agree. I think it is fair to say that I'm honestly not sure that we're doing the right thing here for people using source packages. And if they're using new enough |
|
OK, thanks for the comments all. I'll have another read over tomorrow and we can come to a final decision. |
|
Right, lots going on here. I'm still reading about PEP 518 and such. A couple of points thus far though:
OK, but can we do that in a separate PR please? Should be easily mergeable and the commit history will show Then, I would like us to keep the install warning, at least for now.
Ultimately yes. I think there are many more users than we think about who don't have access to the latest Pythons, or even the option to always fetch from PyPI. I think if we can ensure clear error messages in these cases (and I know they're always going to be under-maintained...) then I think we do a good thing. I'd just like a little more time to be 100% clear, but the scope of this is looking much more manageable. Thanks again for your efforts both. |
That's fair.
Sure. Jon has essentially punted the removal to Django 4.0 by sticking a comment in.
Except That is not to say that this cannot be done earlier, but at least it is on the radar for some point in the future. (I should also add a point that I've missed up until now - it is nice for there to be no "arbitrary" code execution during package builds. Eventually it should also be possible to have a |
Use the SPDX identifier in the license metadata field to clarify that the primary license for Django is the BSD 3-Clause "New" or "Revised" License. The 'License :: OSI Approved :: BSD License' trove classifier is not clear enough to indicate which if the variants of the BSD license Django uses. See django#12013 (comment)
Pull request for license clarification: #12038 |
@carltongibson I've read this and I'm still unsure what the question is. If someone can distill it plainly for me I'm happy to look into it. |
|
@ubernostrum This stems from my #12013 (comment). Read that for more detail. Essentially I noticed when reviewing this that the metadata field for the license was ambiguous. It is often enough to omit this field where it is already available in the trove classifiers. The classifier for "BSD License" isn't specific enough, however, as there are many variants and the value "BSD" in the license field isn't clear either. Hence pull request #12038. That I think is fairly straightforward. So the main issue is that some of the code in Django is copied from the Python Standard Library. We used to have a backport of So the question is: Does this mean Django is released under the terms of both licenses? If so, should we change the license metadata field to be Maybe I'm making something of nothing or opening a can of worms, but I felt this need clarification. (@carltongibson A decision on this doesn't need to hold up this PR. We can address any action separately.) |
|
Look at this again now. I will rebase. |
There was a problem hiding this comment.
OK, thanks both. All still works. 🙂
Having talked to @felixxm, I've split into three commits. We'll backport the purely docs change.
I'll leave this to see if Mariusz has any comments, but looks good. Let's have it! 👍
|
I noticed the |
|
I believe for GitHub to pick up the Co-authored-by tag you'll need to remove the period at the end of the line. Notice that right now Nick's avatar isn't on the commit. |
|
Yes, it was, and |
…up.py. Co-authored-by: Nick Pope <nick.pope@flightdataservices.com>
…cfg. Co-authored-by: Nick Pope <nick.pope@flightdataservices.com>
|
I made tests, few releases, installations etc., and I think we should merge the first two commits and drop the last one with I don't see any advantages (in our case) of |
|
So here is the documentation on how I was trying to be conservative but misunderstood and thought that by not specifying Given all that, perhaps we should just go with: [build-system]
requires = ['setuptools>=40.8.0', 'wheel']
build-backend = 'setuptools.build_meta'I've also notice that we'll need to add a line to So to my understanding, the whole purpose of this PEP 517 & 518 stuff is to formalise how packages are installed and, in the case of source packages, how those are built at install time without As you say, it impacts all installations from source. This is intentional. Is it a negative impact however? Is it actually better because it is less error prone and reproduceable? Besides, how many installations are actually done from source distributions and not wheels these days? If you decide to drop it be aware that this will come back again in the future. |
|
OK, thank again @pope1ni. We can go with the first two changes and pause to think (over the weekend say) about the last.
Yes. It will. That might be OK though: it might not be so hot then 🔥. Anyhow... we'll have a read. 👍 |
|
Let's not worry about the last commit for now. It's not worth getting bogged down in at this stage. I honestly didn't think it would be that contentious or too soon, but I think the real problem here is a lack of clear messaging to developers around packaging in Python - what the roadmap is and how the changes all work. Thank you for taking the time with this 🙃 |
|
Thanks y'all 🎉 |
setuptools allows using
setup.cfgto define a package’s metadata and other options that are normally supplied to thesetup(). This declarative approach avoids the need for some logic insetup.pywhich can help reduce boilerplate and support automation with external tools.Available since setuptools 30.3.0 (8 Dec 2016).
https://code.djangoproject.com/ticket/30948