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

Add discussion "Is setup.py deprecated?" #1331

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

sinoroc
Copy link
Contributor

@sinoroc sinoroc commented Nov 2, 2023

@sinoroc
Copy link
Contributor Author

sinoroc commented Nov 2, 2023

This assumes #1329 gets merged first, which in turn depends on #1330.

I can do the rebases we get there.

I think we have stacked pull requests or something like this, but I do not know how to do it (yet). Sorry for the inconvenience.

@sinoroc sinoroc force-pushed the add-setup-py-deprecated branch 2 times, most recently from a951e6c to eb31cf1 Compare November 2, 2023 17:28
@sinoroc
Copy link
Contributor Author

sinoroc commented Nov 2, 2023

I wrote this based on the questions I have seen on StackOverflow. It is not meant to be exhaustive, and I am sure it can be improved on over time. However imperfect this document is in this state, I think it is important to get such a document published as soon as possible and build from there.

@oscarbenjamin
Copy link

Looking through the updated text and the comments here I think that there are two different purposes being conflated:

  • Writing a page to explain what is or is not deprecated about setup.py and setuptools (what @sinoroc intended here).
  • Writing a guide for updating to modern packaging standards (what many comments including my own were aiming at).

These are two different purposes and I think that it is fine that this page is aimed only at the former. Presumably the intention is that this page could be used as the new URL in an update for this deprecation message:

$ python setup.py bdist_wheel 
running bdist_wheel
running build
running build_py
.../lib/python3.12/site-packages/setuptools/_distutils/cmd.py:66: SetuptoolsDeprecationWarning: setup.py install is deprecated.
!!

        ********************************************************************************
        Please avoid running ``setup.py`` directly.
        Instead, use pypa/build, pypa/installer or other
        standards-based tools.

        See https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html for details.
        ********************************************************************************

!!
  self.initialize_options()
installing to build/bdist.linux-x86_64/wheel
...

For that purpose I think that this page is reasonable.

Separately a porting from setup.py guide is needed and it could be linked from here but this page does not need to be that. Keeping this page small and compact to explain what exactly is or is not deprecated is good if the intention is that the deprecation warning should link to here.

One thing I am not sure about is the statement

No, it is not necessary for a project to have a pyproject.toml file.

This seems like the kind of thing that would end up being out of date and conflicting with future docs in other places. Perhaps at least this could be qualified like

Currently it is not necessary to have a pyproject.toml (because fallback ...). However in future the fallback might be removed from pip etc and so ultimately it is recommended for all projects to adopt at least a minimal pyproject.toml when possible.

Another thing I would add is an explicit statement that setuptools itself is not deprecated and that there is no intention for it to be deprecated in future.

Lastly I would either show a minimal example of a pyproject.toml using setuptools as the backend or at least ensure that a page describing how to do so is being linked to. Ideally this would be a link to a guide for porting from setup.py but if that guide does not exist yet then at least linking to here would be good:
https://packaging.python.org/en/latest/tutorials/packaging-projects/#choosing-a-build-backend

@sinoroc
Copy link
Contributor Author

sinoroc commented Nov 3, 2023

Thanks a lot for the comments. Hopefully tomorrow, I will take some time to re-read all comments carefully. My intention is to rewrite in order to tighten the focus on the "deprecation of setup.py as a CLI tool". And if no one has done it yet, I intend to start a different page for the "How to modernize a setup.py based project?" as there seems to be some kind of consensus that it is what we should do.

@oscarbenjamin
Copy link

Assuming that this goes with #1341 I think that what is here is fine. I would just remove the section about pyproject.toml from this page so that it is purely focused on what is deprecated and what the direct alternatives are supposed to be.

@sinoroc
Copy link
Contributor Author

sinoroc commented Nov 5, 2023

So #1341 has been started as a companion for this PR. So some bits can be moved to that other page.

Here are some of the latest updates:

  • added note that setuptools itself is not deprecated either (obviously)
  • added section about custom setup.py commands being deprecated, and list of tools that can be used as replacement: nox/tox, make, pyinvoke, and more
  • added section about custom build steps NOT being deprecated.
  • removed some things that are better expressed in that other page, I can not put links yet, but once both are merged we can add the links between those 2 pages
  • kept the note that pyproject.toml is not necessary but STRONGLY recommended with a minimal example of build-system table (later we can link to the other page)

Of course, everything can be rephrased, reworded, reordered, clarified, but I guess I might be running out of steam (already? :D ), so maybe it is better to get this merged so that others can take over. There is clearly a bunch of things that have been commented here, but I either do not understand or do not know how to phrase.

Copy link
Contributor

@jeanas jeanas left a comment

Choose a reason for hiding this comment

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

Thank you for your work, @sinoroc!

@webknjaz
Copy link
Member

webknjaz commented Nov 6, 2023

(Perhaps you could click "resolve" on the threads you addressed so it's easier to see what remains?)

Right, I never know what the etiquette is around this. Should I "resolve" the thread or should the person who opened the thread resolve it when they are satisfied with the outcome?

@sinoroc so the guidance I follow and recommend other people do is:

  1. Generally, the thread author should be the one to resolve it. This is because other people may not interpret the request, and ultimately only the author knows what they meant. Also, the author would be able to accurately judge whether the reaction (change in code or response comment) is adequate and solves their concern.
  2. If a reviewer suggested to change something and the PR author did that, it's a good tone to add a comment to the thread stating this and linking the commit. Note that this might not work well if the PR submitter exercises the force-push strategy, which is one I've seen some project ask that only new commits are added, making the incremental updates discoverable more easily.
  3. Clicking Resolve does not generate notifications (like emails). When this is done, the thread author would easily lose the reference to their thread, and it's often hard to find, once it's collapsed. For this reason, I typically recommend posting a comment saying “Resolving this thread.” and maybe some reasoning, and only then, actually clicking Resolve. This increases transparency (it's not obvious when things got “resolved” otherwise) and allows for people to find the way back to those threads, using email notifications which contain direct links…

@webknjaz
Copy link
Member

webknjaz commented Nov 6, 2023

I would say that if you are confident that the other person would consider it resolved then mark it as resolved.

I think people have different opinions on this. Unless the interaction is of the form "I think you should do X"/"OK, I did that", I tend to prefer to leave it to the person who made the original comment to confrm that they are happy by being the one to mark the conversation as resolved. And I prefer it if others do that when I'm the commenter. But ultimately, I think there's no standard etiquette here - do what you feel is best.

+1

The other day, somebody introduced me to https://conventionalcomments.org in another PyPA repo, I think. It seems like this is something that could improve the communication even more.

Of course, everything can be rephrased, reworded, reordered, clarified, but I guess I might be running out of steam (already? :D ), so maybe it is better to get this merged so that others can take over.

+1 - people can make improvements in follow-up PRs. Let's not burn people out by making it a huge task to make an incremental improvement.

Yeah, a few days ago I merged a PR (jazzband/pip-tools#1681) that took 13 months to complete, with hundreds of review comments (my fault partially). I'm pretty sure it was exhausting to all the parties involved.

I'll try to take a prompt look at this PR since I haven't had a chance to review properly and will try to get it merged unless there's a noticeable problem.

@webknjaz
Copy link
Member

webknjaz commented Nov 6, 2023

Aha… So the CI is failing with:

/home/runner/work/packaging.python.org/packaging.python.org/source/discussions/setup-py-deprecated.rst:129: WARNING: term not in glossary: 'Build Frontend'
/home/runner/work/packaging.python.org/packaging.python.org/source/discussions/setup-py-deprecated.rst:129: WARNING: term not in glossary: 'Build Backend'

(https://github.com/pypa/packaging.python.org/actions/runs/6764239357/job/18382296998?pr=1331#step:5:62)

@webknjaz
Copy link
Member

webknjaz commented Nov 6, 2023

So this should be merged together with #1329 and it'll be fine...

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

@sinoroc I'm going to merge this, but I saw some punctuation and grammar mistakes in the text. Would you mind feeding it to LanguageTool and/or Grammarly and sending the fixes as a follow-up?

@EpicWink
Copy link
Contributor

EpicWink commented Nov 6, 2023

Perhaps you also want to change list-item 2 of the doc for setup.py

[setup.py is] the command line interface for running various commands that relate to packaging tasks. To get a listing of available commands, run python3 setup.py --help-commands.

@webknjaz
Copy link
Member

webknjaz commented Nov 6, 2023

@EpicWink that sounds like another follow-up. This PR is about a "discussion" document, while your suggestion concerns a "guide".

@webknjaz webknjaz added this pull request to the merge queue Nov 6, 2023
@webknjaz webknjaz added type: enhancement A self-contained enhancement or new feature component: discussions labels Nov 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to Branch Protection failures Nov 6, 2023
You're not authorized to push to this branch. Visit "About protected branches" for more information.
@sinoroc
Copy link
Contributor Author

sinoroc commented Nov 6, 2023

@webknjaz

I saw some punctuation and grammar mistakes in the text. Would you mind feeding it to LanguageTool and/or Grammarly and sending the fixes as a follow-up?

Thanks for the suggestion, done

Also rebased on main so that we have working links to the build backend and frontend glossary definitions.

And fixed the link to setuptools page.


Later...

Rebased again, to check against the Furo theme, and also I added some usage of the :file: role here and there.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

A couple more minor things.

source/discussions/setup-py-deprecated.rst Show resolved Hide resolved
source/discussions/setup-py-deprecated.rst Show resolved Hide resolved
@pradyunsg pradyunsg added this pull request to the merge queue Nov 8, 2023
Merged via the queue into pypa:main with commit 2ff089b Nov 8, 2023
4 checks passed
@sinoroc sinoroc deleted the add-setup-py-deprecated branch November 8, 2023 20:43
@oscarbenjamin
Copy link

Nice work @sinoroc !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: discussions type: enhancement A self-contained enhancement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet