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

docs: add note about opting into PEP 621 #1065

Merged
merged 7 commits into from Apr 1, 2022
Merged

Conversation

abravalheri
Copy link
Contributor

Closes #1064

This is just a quick suggestion covering the bare minimum without touching the other parts of the doc.

You guys might want to do it in a completely different way. In that case, please feel free to ignore the PR.

@joerick
Copy link
Contributor

joerick commented Mar 27, 2022

Thanks again for the PR. On this change, what would the minimum fields be that we could show in this example? It would be nice to just include the minimum, that's kinda what the example was trying to do before (hence why it includes the [build-system] section.

I had a read of the PEP, is the minimum just name = "..."? or does it also need version = "..."? Of course we'd also link to the PEP with the full list, but a minimal example is always good to give users a starting point.

@abravalheri
Copy link
Contributor Author

The PEP requires all the projects to specify name statically and version either statically or dynamically.

However the PEP also forbids setuptools from backfilling using external sources (e.g. setup.py) the values of any attribute that is not listed under dynamic. This may be difficult to deal with.

@abravalheri
Copy link
Contributor Author

abravalheri commented Mar 27, 2022

Version v61.2.1 of setuptools will use the values from setup.py anyway when nothing is found in pyproject.toml [project]. However there will be a bunch of warnings. I added this provisional behaviour just now, so we don't break the builds while users are getting used to he consequences of PEP 621.

@joerick
Copy link
Contributor

joerick commented Mar 27, 2022

Ahh, I see. So, I had a go at making a bigger improvement to this section. If you have time, it would be great if you could let me know what you think. Probably easiest to read the rendered output - https://cibuildwheel--1065.org.readthedocs.build/en/1065/options/#requires-python

Copy link
Contributor Author

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Hi @joerick thank you very much for the changes.
In general it does look good to me.

docs/options.md Outdated
```

- If you didn't already have a `pyproject.toml` that had a `[project]`
table, you should migrate values from `setup.py` or `setup.cfg`, or
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you should migrate

My only remark is that support for PEP 621 was just added last Friday, so there might be sharp edges left.


The following is just maintainer babbling 😅:

We can actively start encouraging people to migrate to it, however in setuptools we decided to keep the experimental status for a while to guarantee that we have the freedom to modify how we implement it before committing to an specific design.

Changes in interface will not occur in the [project] table (after all it is standardized), but if the users reach out for [tool.setuptools] there is no guarantee the fields there will stay unchanged between versions.

If in the following days we receive reports that the release seriously break important packages for the community we might also consider yanking it and backing off a bit while we re-implement any problematic bits.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for reviewing !

My only remark is that support for PEP 621 was just added last Friday, so there might be sharp edges left.

Hm. Yes okay I see. So what's the conservative route for users with a [project] table? Would listing options in dynamic be the safest way to go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the users specify as dynamic the fields that they are providing via setup.py, that would be safe.

I think the parts of PEP 621 important to keep in mind are:

Build back-ends MUST raise an error if the metadata specifies a field in dynamic but the build back-end was unable to provide the data for it.

Build back-ends MUST raise an error if the metadata specifies a field statically as well as being listed in dynamic.

If the metadata does not list a field in dynamic, then a build back-end CANNOT fill in

@deepika094
Copy link

@abravalheri so if i had pyproject.toml file with project tag where i was just defining name and python-requires and for version i had a dynamic logic in setup.py , now setuptools will complain -
ERROR:setuptools.config.pyprojecttoml:configuration error: project must contain ['version'] properties

[06:26:51] [Step 3/5] ############################
[06:26:51] [Step 3/5] # Invalid pyproject.toml #
[06:26:51] [Step 3/5] ############################
[06:26:51] [Step 3/5]
[06:26:51] [Step 3/5] Any configurations in pyproject.toml will be ignored.
[06:26:51] [Step 3/5] Please note that future releases of setuptools will halt the build process
[06:26:51] [Step 3/5] if an invalid file is given.
[06:26:51] [Step 3/5]
[06:26:51] [Step 3/5] To prevent setuptools from considering pyproject.toml please
[06:26:51] [Step 3/5] DO NOT include the [project] or [tool.setuptools] tables in your file.

To fix this i should change it to below?-

[project]
name = "***"
dynamic = ["version"]
requires-python = ">=3.7" #Python version

@abravalheri
Copy link
Contributor Author

abravalheri commented Mar 28, 2022

Hi @deepika094, yes that change makes sense.

If you are using any other field in setup.py that has a correspondence in pyproject.toml [project], you should include them in dynamic as well1.

For example let's say you define version, install_requires and extras_require in setup.py.
Then you should have dynamic = ["version", "dependencies", "optional-dependencies"].
Please note that if you define something in dynamic that cannot be found by setuptools, there might also been an error.

These principles are defined in the PEP 621:

Build back-ends MUST raise an error if the metadata specifies a field in dynamic but the build back-end was unable to provide the data for it.

If the metadata does not list a field in dynamic, then a build back-end CANNOT fill in

Please note that this maintenance overhead is a direct consequence of opting into PEP 621.
If that is too much for the project, the alternative is to remove [project] from pyproject.toml and rely on the other mechanisms listed in the cibuildwheel documentation to specify which versions of Python are supported.

Footnotes

  1. PEP 621 lists the corresponding fields between setuptools and pyproject.toml.

@deepika094
Copy link

@abravalheri thank you. that's very helpful. Indeed we define lots of fields in setup.py like install_requires, extra_requires, entry-points so I guess we will have to make change in all the projects and make these fields dynamic in pyproject.toml.
@abravalheri @joerick If we look for alternative and we remove [project] from pyproject.toml and for name , version and python-requires we keep all these just in setup.py
eg.
Before-
setup(
name=pyproject_toml['project']['name'],
version=_determine_version(),
python_requires=pyproject_toml['project']['requires-python'],
....

After-
setup(
name='myproject_name,
version=_determine_version(),
python_requires= ">=3.7",

will that be ok and compliant with pep621? in that case we can just keep pyproject.toml to have below and not [project]-
[build-system]
requires = [
"setuptools >= 46.1.0",
"wheel",
"toml"
]
build-backend = "setuptools.build_meta"

Or the recommended way is to use [project] and for fields we define in setup.py we should mark them as dynamic in pyproject.toml?

@CAM-Gerlach
Copy link
Contributor

CAM-Gerlach commented Mar 29, 2022

Personally, while I'm a big fan of PEP 621 (and am thrilled by @abravalheri 's work there) and look forward to all tools supporting it, IMO cibuildwheel strongly pushing users to use it without a full, in-depth explanation of the consequences, caveats and complexities. seems a bit premature—experimental Setuptools support just landed, and Poetry still doesn't support it with no clear timeline for when it will be added—whereas there are serious implications users need to understand before they switch to it or they are in for a fair amount of trouble (as was the case for Black when they required pyproject.toml for configuration with no alternative, which triggered PEP 517 builds on unprepared projects).

Furthermore, it means that cibuildwheel has to independently keep up with the rapidly changing real-world situation thereof as tools implement and iterate on it, or otherwise risk giving users bad advice, which seems to be a non-trivial mantience burden for a package not specifically focused on this part of the ecosystem (project source metadata).

Since this is just about adding a single, simple metadata key as a way for cibuildwheel to better infer the Python versions supported by a project, I'd suggest not bringing in the whole big issue of PEP 621 migration into it, at least for now, and instead just list the different mechanisms in order of preference, mentioning that users should use the highest one they are currently using, and linking to the authoritative documentation and specifications for more detail on each.

Also, on another note, since it has been accepted, PEP 621 itself is not the canonical specification or its documentation and thus should not be linked to in that capacity, as opposed to the official specification on the packaging guide. The latter can and will be updated over time as the specification changes (based on future PEPs and minor errata); PEP 621 is already out of date in that respect and will grow more so over time.

docs/options.md Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor

This is heavily my fault - I added this (about a year ago, maybe more) with the understanding that the PEP 621 location (project.requires-python) was the canonical location for this information (as stated in the PEP), and I was hoping to avoid a proliferation of locations for this information. I hadn't really thought about someone only adding this one value.

When we later added tool.cibuildwheel support, I explicitly avoided adding this field, as it was supposed to come from the one of the other sources. It's really optional for cibuildwheel - it just helps cibuildwheel discover the correct minimum - it was very important when we were still supporting Python 2 to give Python 3 users a much nicer experience. (And still helps for Python 3.7+ or 3.8+ users who don't want to be bothered with our following of Manylinux's ubuntu-18.04-inspired support for 3.6+). But I think that's when the emphasis causing just this value to be filled came. I certainly was not thinking about consequences of just adding that one value. We never were pushing for full PEP 621 because there were no backends for it yet.

Something similar happened with metadata.license and setup.cfg, with projects filling in just that one field in setup.cfg. PEP 621 is not the same, though, in that it's all-or-nothing; as a benefit to tools wanting to statically infer information from pyproject.toml, it requires everything to either be specified or listed explicitly in dynamic.

My thought would be (I think closely following @CAM-Gerlach's suggestion above):

  • Try to remove the emphasis on the PEP 621 location, and just state that this is supposed to be discovered, and the environment variable is available if the discovery doesn't work.
  • Point out PEP 621 backends that support building (currently setuptools (experimental) and masonpy, hopefully scikit-build in the near future).
  • Fix the links to the PEP 621 docs.
  • Maybe support this being specified in tool.cibuildwheel. I'm still biased toward not allowing it, though.

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@joerick
Copy link
Contributor

joerick commented Mar 29, 2022

Thanks for the input, all. I still think that adopting the project.requires-python field for this option was the right thing to do - it's an interoperability spec after all - but I totally get that for good, practical reasons, this has ended up a more complicated picture than we originally thought.

I'll redraft the docs to be more in line with the discussion here. (And I'd agree with @henryiii, I don't think we should add a tool.cibuildwheel option for this, because in the fullness of time, that option isn't necessary.)

@abravalheri
Copy link
Contributor Author

Thank you very much @joerick , @CAM-Gerlach and @henryiii .

Please feel free to make any changes or even ignore the PR. My original intention was just to draw attention of the users that adding [project] might have unintended consequences.

I am very sure that the original intention of the text was spot on. Maybe this is just a case of Hyrum's Law applied to documentation.

@CAM-Gerlach
Copy link
Contributor

Thanks for the input, all. I still think that adopting the project.requires-python field for this option was the right thing to do - it's an interoperability spec after all - but I totally get that for good, practical reasons, this has ended up a more complicated picture than we originally thought.

Just to be clear, supporting the project.requires-python key and giving it precedence over the others was definitely the right thing to do, and in fact a bit part of the intention of the standardized project table was to allow other tools to statically read the project source metadata from a common location without having to parse and extract it from each tool's bespoke configuration format. My comments were just about how the documentation was framed and worded around that in a way that eased the effort and issues surrounding it for both you and your users, which is sounds like we all pretty much agree on 👍

docs/options.md Show resolved Hide resolved
docs/options.md Outdated Show resolved Hide resolved
docs/options.md Outdated Show resolved Hide resolved
docs/options.md Outdated Show resolved Hide resolved
joerick and others added 2 commits March 30, 2022 09:13
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
@henryiii
Copy link
Contributor

The pycln failure (due to typer) is fixed in #1066.

@joerick joerick merged commit 481f2eb into pypa:main Apr 1, 2022
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.

[Docs] Example may accidentally be encouraging users to write invalid pyproject.toml files
5 participants