-
Notifications
You must be signed in to change notification settings - Fork 430
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
Running setup.py directly is deprecated #737
Running setup.py directly is deprecated #737
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@@ -265,14 +265,13 @@ You can install it by doing the following : | |||
|
|||
.. code-block:: bash | |||
|
|||
$ python3 -m venv .venv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure venv management should be covered here. There are tons of ways of to manage a venv, and forcing one in a tutorial ends up badly in my experience. Just a mention of a venv should be enough, as is before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Python's built-in way of creating and enabling a venv. Users are free to choose alternative methods but I prefer explicit steps here to document what we know works and avoid issues being opened around implicit steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer explicit steps here to document
This can be easily paraphrased as "pushing an opinionated tooling", which worries me. IMO, for anything related to venv, considering how every detail of that topic is open for a million different debates, it's better to keep an equal distance to all options.
Of course, you can ignore my comment; it's not a big thing, but I believe these types of details are more important than most people care for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opinionated tooling would have been a suggestion to use uv
. 😄
I do tons of responding to Node.js and node-gyp issues. It is there (and elsewhere) that I have learned the hard way that incomplete directions and assumptions lead to long-winded discussions.
it's better to keep an equal distance to all options.
I think suggesting the one that is built-in and requires zero third-party software is relatively safe.
Installing the current default branch | ||
------------------------------------- | ||
|
||
$ python3 -m venv .venv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: #737 (comment) With these changes, it looks like the steps are more about venvs, not the actual goal of the doc.
@@ -2,7 +2,7 @@ | |||
|
|||
import pytest | |||
# we have to import the pytest plugin fixtures here, | |||
# in case user did not do the `python setup.py develop` yet, | |||
# in case user did not yet `pip install ".[develop]"`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make better sense with "yet" at end of the sentence, as the before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It felt like a dangling participle when I read it. (But I would bet dollars to doughnuts that it is not a participle. 😃)
OK... This is now ready for review. The only thing I am unclear about is the |
I went to look into setuptools' changelog what these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I like this idea although I've never used the test pypi index before. @cclauss I'm up for testing it if you're interested and if it works deploy to normal pypi afterwards. |
Co-authored-by: Mathieu Dupuy <deronnax@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked out the PR to test and I got a problem:
make
Makefile:68: *** missing separator. Stop.
On line 68, you used space for indentation instead of tabs. Makefile only accepts tabs for indentation.
Even with the above fixes, the steps in the README do not work for me locally:
However, this is also true on the |
Co-authored-by: Adam Weiss <amweiss@users.noreply.github.com>
Co-authored-by: Adam Weiss <amweiss@users.noreply.github.com>
What you are posting is seemingly a docker error, and there is no docker in the Makefile, so I am not sure how you got it. For the Makefile, I was able to have Regarding the docker error, it's something trivially wrong in the dockerfiles: the Dockerfiles ask for a tagged base image that is never generated, hence the error. It probably never worked. |
This PR changed the Dockerfile, so I wanted to see what it did. However, it seems like it's never worked. |
Co-authored-by: Mathieu Dupuy <deronnax@gmail.com>
as the oldest maintainer of the project, I would like to review it in detail before merging. hope you are OK with it. |
Please be aware that the release for Django 5 support is blocked on this PR: #680 From my understanding this PR is also ready already, even if not everything is perfect. Many are expecting a release soon, IMHO I believe we should proceed - let me know what I can do to accelerate the merge, how may I help? P.S |
I also agree that a new release with Django 5.0 support is needed, but I don't think this PR is a blocker. |
Looks good to me. Migrating the setup.py to a declarative form could be the next step, wdyt? |
With these changes in place, if you want to make the README work correctly or the docker compose actions work, the following changes are still needed. Add the following lines:
so the
After that, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked for me now.
This sat un-reviewed to too long until
Let's get some maintainers reviews on this please. |
Co-authored-by: David Hotham <david.hotham@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job!
While this is fresh in everyone's head, would now be a good time to make a new release? The change in setuptools was reverted, but it's been deprecated for years, so it will be removed at some point in... Would be nice to give plenty of time for upgrading. Thanks! |
Running
python3 setup.py
directly as a script is deprecated.Let's use
python3 -m build
andpython3 -m pip install
instead of directly runningsetup.py
.