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

Update pyproject.toml #204

Merged
merged 9 commits into from
Feb 29, 2024
Merged

Update pyproject.toml #204

merged 9 commits into from
Feb 29, 2024

Conversation

VladimirFokow
Copy link
Contributor

@VladimirFokow VladimirFokow commented Feb 3, 2024

pyproject.toml is outdated. I have:

  • moved the [build-system] to the start of the file (because this setting is pretty important, and I saw this everywhere in examples in the docs)

  • simplified & updated the [build-system] table

  • removed all # Optional comments (because the majority of keys are optional by default) - it's better to mark only the # REQUIRED ones

  • added useful explanations and links

(plus, integrated the changes from pull request #197)


Explanations:

If you are happy with the current edits, no need to read all this!

If you're not, here are my motivations for EACH change.
Note:
These changes improve the understandability for the end user (like me) a lot. And they fix all outdated things. This PR has one goal: to update pyproject.toml.


click: Explanations
  • How I changed the [build-system] table:

    The [build-system] parameters that I used here are:

    [build-system]
    requires = ["setuptools"]
    build-backend = "setuptools.build_meta"
    • why setuptools? - it is the default build backend
    • why these parameters? - this is the minimal valid example of parameters that I could find in the Docs

    Pros of these new [build-system] parameters:

    • Move away from specifyingthe assumed default build requirements from pip:
      • the current "assumed default build requirements from pip" differ from the ones specified in this file. Fact: this repo is rarely maintained. Minimal example -> no need to maintain so often -> no inaccuracy.
      • the default pip parameters - are needed for legacy compatibility. Instead of recommending the (legacy) default pip build parameters, this file can have the minimal default recommendation for new projects.

    Cons of these new [build-system] parameters:

    • doesn't specify the min. version of "setuptools" -> an error will NOT (?) be reported if the available version of setuptools is not recent enough. -- (Is this a big problem?) Maybe some min. version should be specified - which one?


  • Why I removed the "# Optional" comments:
    The only required keys are: name and version. All the other ones are optional.
    So it's better to not mark every key as Optional but instead only to mark these 2 as # REQUIRED.
    • no future contributors forget to mark something as optional
    • the user understands the general rule (that the majority of keys are optional)
    • the user will check the up-to-date list in a linked specification - so no need to maintain these tags (which is a proved problem: it went for 6 months just now with a PR hanging for fixing the incorrect labels)
    • DRY principle

Other quotes and proofs


  • proof why there is no # REQUIRED comment next to the [build-system] table:

A pyproject.toml file may be used to store configuration details other than build-related data and thus lack a [build-system] table legitimately.


Should pyproject.toml be added?
A pyproject.toml file is strongly recommended. The presence of a pyproject.toml
file itself does not bring much. What is actually strongly recommended is the
[build-system] table in pyproject.toml.


Choosing a build backend:
Both of these values will be provided by the documentation for your build backend,
or generated by its command line interface.


Usually, you’ll just copy what your build backend’s documentation suggests.
There should be no need for you to customize these settings.


Please correct this if it's wrong done

What comment is suitable if the build-backend parameter is missing?

About the build-backend parameter:

If not defined, then the fall-back behaviour is to use setuptools.

I've found this in only one place - in a note in parenthesis; I have doubts: if pyproject.toml exists and requires = NOT setuptools but build-backend isn't defined, will setuptools still be used? - or the chosen backend?

On the other page, I've seen:

If the table is specified but is missing required fields then the tool should consider it an error

So the other build backend chooses what to do? - then it's not setuptools in this case?

..unrelated, but still:

Tools should not require the existence of the [build-system] table

- I'm not knowledgeable enough to know, how it is possible to use another build backend if the [build-system] doesn't exist.. Does it make sense to write here that [build-system] is optional? Can any build systems exist that can work without the [build-system] table?

You can just remove this line please:

"# If not defined, then the fall-back behaviour is to use setuptools."

and maybe give this key a "# REQUIRED" comment.. or not?


Plus, maybe add this as well?

Note that the presence of a pyproject.toml file (even if empty) triggers pip to change its default behavior to use build isolation.
https://packaging.python.org/en/latest/guides/modernize-setup-py-project/#where-to-start


  • quote what the default build backend is:

if there is no pyproject.toml file or the build-backend parameter is not defined, then the fall-back behaviour is to use setuptools.

- extend descriptions of build-system table
- add useful links
- edit other comments
- be more clear that the values in [build-system] may be generated by the command line interface of the build backend, so the [build-system] table is not required
- reorganize comments to better correspond to their sections
add the changes authored by thomasbbrunner in pypa#197
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented Feb 3, 2024

@VladimirFokow almost everything you changed is really optional. Also, I recommend making small atomic changes so that they could be reviewed more easily and accepted faster, without implying countless rounds of reviews.

@webknjaz
Copy link
Member

webknjaz commented Feb 3, 2024

@di could you approve the CI run? I don't have that privilege in this repo.

@VladimirFokow
Copy link
Contributor Author

VladimirFokow commented Feb 3, 2024

Thanks for the reviews!

Sorry for so many changes.. The total file isn't large though; if people now add their commits on top, we can all move closer towards a good final file.

  • yes, everything is optional, so maybe it's easier to say it one time, and only mark the required stuff as required?) this is the last point in my "Notes and info for the reviewers:"

@webknjaz
Copy link
Member

webknjaz commented Feb 3, 2024

The total file isn't large though; if people now add their commits on top, we can all move closer towards a good final file.

Yes, which may mean months in-between reviews. Each asking to change one line. And the entire patch not being acceptable for a year.

It's not about the size of the patch but semantically different changes. Does this PR mean to apply arbitrary updates that aren't related to each other? That doesn't sound like a sustainable strategy to me. Combining formatting and functional changes in the same patch usually makes it way easier for the reviewers to get lost in what's happening, which in turn prompts them to postpone doing reviews or delegate to somebody brave.

I have a few collections or links related to contributing PRs. Perhaps, you'll find some of them useful: https://gist.github.com/webknjaz/2aa9c7a43b51c1385260ff87e0cbb9e3 / https://gist.github.com/webknjaz/a7362787a80067af8621a85a71746ca1 / https://gist.github.com/webknjaz/cb7d7bf62c3dda4b1342d639d0e78d79 (and https://gist.github.com/webknjaz/a5a4fb374b7579de827e6bedb93a5220 which is mostly dedicated to advanced Git usage, so maybe isn't applicable to you).

  • yes, everything is optional, so maybe it's easier to say this one time, and only mark the required stuff as required?) this is the last point in my "Notes and info for the reviewers:"

I don't know. Other people may have opinions. I only wanted 9 bytes removed from this file.

@jeanas
Copy link

jeanas commented Feb 3, 2024

I've found this in only one place - in a note in parenthesis; I have doubts: if pyproject.toml exists and requires = NOT setuptools but build-backend isn't defined, will setuptools still be used? - or the chosen backend?

It is setuptools. This is legacy behavior that originates in the history of these fields — build-system.requires was initially defined (in PEP 518) before build-system.build-backend (which is from PEP 517). Therefore you may encounter rare (?) pyproject.toml files with only build-system.requires. This information is on https://packaging.python.org/en/latest/specifications/pyproject-toml/, but I don't think we should put it anywhere else. Users want to know what they should do, not the finer details of how tools keep compatibility with deprecated practices.

Edit: However, if build isolation is used (the default in pip) and setuptools is not in the requires list then this will simply give an error about not being able to import the backend.

On the other page, I've seen:

If the table is specified but is missing required fields then the tool should consider it an error

So the other build backend chooses what to do? - then it's not setuptools in this case?

The build frontend should raise an error. No build backend is invoked.


General feedback:

Imagine how I feel when there are 7 different places to read about python packaging)

For this very reason, I would honestly just remove the descriptions of the fields and only put a link to a subheading on the pyproject.toml guide above each field (e.g., https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#python-requires).

@webknjaz
Copy link
Member

webknjaz commented Feb 3, 2024

Edit: However, if build isolation is used (the default in pip) and setuptools is not in the requires list then this will simply give an error about not being able to import the backend.

I remembered I was updating a related explanation in pip docs recently (pypa/pip#12449) and here's exactly what it does: https://pip.pypa.io/en/stable/reference/build-system/pyproject-toml/#fallback-behaviour. The exact implementation of the decision tree is @ https://github.com/pypa/pip/blob/811beab/src/pip/_internal/pyproject.py#L29-L179.

@webknjaz
Copy link
Member

webknjaz commented Feb 3, 2024

I would honestly just remove the descriptions of the fields

This has its ins and outs. My understanding is that people may use this repo as a starting point and would be filling out the files as templates. In such a case, keeping the context close and not sending them on the round trip to the external docs has the least mental overhead. OTOH, keeping the descriptions in sync and up-to-date causes a burden for the maintainers.

I'm not going to make a judgement call, but as this repo doesn't get a lot of activity, it does feel like reducing said maintenance burden would be beneficial in the long run.

@VladimirFokow
Copy link
Contributor Author

VladimirFokow commented Feb 3, 2024

I actually didn't mean that the descriptions in this file were a burden - I think the descriptions here are good.

That said, I also don't mind removing them completely if there is a specification link at the start with them all.

Thinking about the larger picture -

I think in this file specifically, there can be quick comments about why we decided to include these specific keys for the example project, even though they are optional, they are good to have, because ...

@webknjaz
Copy link
Member

webknjaz commented Feb 3, 2024

I actually didn't mean that the descriptions in this file were a burden - I think the descriptions here are good.

You're talking from the PoV of a consumer / end-user. We're taking a point of a maintainer who ends up having to update those description regularly. It's a kind of a responsibility that tends to pile up and contribute to people burning out.

@VladimirFokow
Copy link
Contributor Author

VladimirFokow commented Feb 3, 2024

To sum up:

1.‎‎‎‎‎‎‎‎ done✅ : I can create a commit to:

  • Use this simple comment near build-backend - to say to the user what happens if it's missing:
    # If not defined, then legacy behavior can happen.

  • fix spelling to: "behavior"


2. [optional]
Create new SEPARATE issues to: - choose the set of keys as a default quick start - have only those descriptions here that are already in the docs (or summarise the docs)

(so if we want to add something new here - add it to the docs first, then to here)

  • remove comments if: obvious / unnecessary / too long
  • add useful quick summarisations to this file from the docs

Does everyone agree for me to do these 2 actions?

Should some min. version of "setuptools" be specified - which one?

@pfmoore
Copy link
Member

pfmoore commented Feb 3, 2024

Does everyone agree for me to do these 2 actions?

I don't think we need a PR to change the spelling of "behaviour". There's no policy that American spelling should be used over British spelling, so leave existing spellings alone (although I thought the spelling correction was just a change to something you'd added in this PR, not a change to existing text?)

As for the second action, I have the same reservations as @webknjaz - I don't think it will add enough benefit to justify the maintenance effort. Simply reading the discussion on this PR has been more work on this repository than I've otherwise done in months - this project is supposed to be (very) low maintenance. But I'm largely indifferent to the idea, if you want to make a PR and someone wants to merge it, I won't object. I just don't personally think it's that important.

@VladimirFokow
Copy link
Contributor Author

VladimirFokow commented Feb 3, 2024

@pfmoore (yes, I've introduced "behaviour" here, then "behavior" was suggested)

I just want to improve the current pyproject.toml because it's too outdated.
I am happy to quickly apply a minimum set of changes that everyone agrees on.

@sinoroc
Copy link

sinoroc commented Feb 4, 2024

Thanks a lot @VladimirFokow for working on this pull request :)

- separate links to guide and specification
- remove text that can already be found by following the provided links
- more accurate comment about the `build-backend` key
@VladimirFokow VladimirFokow marked this pull request as ready for review February 4, 2024 13:10
@VladimirFokow
Copy link
Contributor Author

VladimirFokow commented Feb 4, 2024

Implemented the suggestion
The only concern that I have remaining is:

Should some min. version of "setuptools" be specified? If so, which one?

@VladimirFokow
Copy link
Contributor Author

VladimirFokow commented Feb 4, 2024

Same tests issue happens here that was in #197..

eps = importlib_metadata.entry_points().get(self.namespace, ())
AttributeError: 'EntryPoints' object has no attribute 'get'

Any ideas what the next steps should be?

pyproject.toml Outdated Show resolved Hide resolved
@chrysle
Copy link
Contributor

chrysle commented Feb 4, 2024

Any ideas what the next steps should be?

The deprecated entry point interfaces for importlib_metadata have been removed in v5. I think we should pin that on < 5 with an environment marker for Python 3.7.

@VladimirFokow
Copy link
Contributor Author

VladimirFokow commented Feb 4, 2024

Is this the issue?:

I've attempted to engage with the maintainers of that project, but they refuse to engage and dismiss my proposals without suggesting alternatives.

Unfortunately, I don't have the technical skills now to figure out a solution to this.


Python 3.7 has reached its end-of-life What if we just drop support for Python 3.7?
See PR #207

@chrysle
Copy link
Contributor

chrysle commented Feb 4, 2024

Python 3.7 has reached its end-of-life What if we just drop support for Python 3.7?

Yes, but in a separate PR I'd suggest. For now, it's mostly about pinning the importlib_metadata requirement in tox.ini:

deps =

@sinoroc
Copy link

sinoroc commented Feb 4, 2024

Should some min. version of "setuptools" be specified? If so, which one?

I would say no.

It might trigger a whole (long) discussion of its own, so I would not push too hard on this topic within this pull request if I were you.

@jeanas
Copy link

jeanas commented Feb 4, 2024

Should some min. version of "setuptools" be specified? If so, which one?

I would say no.

It might trigger a whole (long) discussion of its own, so I would not push too hard on this topic within this pull request if I were you.

IMHO, this is easy to answer: the setuptools documentation does not have a bound, so this sample shouldn't either.

@di di merged commit 5d27795 into pypa:main Feb 29, 2024
15 checks passed
@webknjaz
Copy link
Member

Should some min. version of "setuptools" be specified? If so, which one?

I would say no.

The same considerations should be applied as with normal software deps. If a project uses a specific feature from a lib, it's natural to declare that a minimum version of that lib is required. Similarly, if there are features used in the packaging config that setuptools only supports starting with a certain version, it's good to communicate that. I recommend pairing that with an explanatory comment like # feature X was first implemented in setuptools vY and so on.

But outside of such context, when the features used are implemented by any version of a dependency, then the dependency declaration should communicate exactly that — an unbound requirement specifier.

Upper bounds are more complicated and shouldn't generally be used most of the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants