pkgs/sagemath-standard: Move metadata from setup.cfg to pyproject.toml #37902
Conversation
|
I'm counting the votes from #36951: 👍 @jhpalmieri @mkoeppe @kwankyu @kiwifb (I am not the author of this PR, I didn't vote yet.) |
|
Documentation preview for this PR (built with commit 443b5bb; changes) is ready! 🎉 |
|
I'll repeat my commment #36951 (comment) since I'm not opposed to the idea of this PR, but only to its current execution. I'm -1 on this for several reasons:
|
This comment was marked as abuse.
This comment was marked as abuse.
|
I am -1 for now because I think #36982 is the better approach. But if that fails I would reconsider. |
|
+1: Moving from 👍 @jhpalmieri @mkoeppe @kwankyu @kiwifb @NathanDunfield |
|
I agree that generating pyproject.toml with m4 is better than generating setup.cfg with m4. It doesn't make sense to force the use of setup.cfg, and allow generating it with m4, in order to avoid generating pyproject.toml with m4. The issue of how to generate pyproject.toml seems to me to be separate from the issue of whether tu use pyproject.toml. Perhaps the scheme in #36982 is better than using m4. If so then it should be possible to adopt that scheme later. +1 |
|
I've reworked this on top of
This is no longer a trivial PR. Also fixes the breakage noted in #37894 (comment) |
|
This PR now does not seem to be the same PR which was disputed. If no one would object, I will remove the "disputed" label. |
| description = "Sage: Open Source Mathematics Software: Standard Python Library" | ||
| dependencies = [ | ||
| 'six >=1.15.0', | ||
| # From build/pkgs/sagelib/dependencies |
There was a problem hiding this comment.
These "section header" comments now make less sense and are better be replaced by proper ones (or deleted completely)
There was a problem hiding this comment.
Could you please expand on this? For example, build/pkgs/sagelib is downstream of sagelib in src; why would then the later refer to the former and not the other way around?
There was a problem hiding this comment.
The comments explain where the dependencies are coming from; the audience for these explanations: people familiar with the build system.
There was a problem hiding this comment.
When pyproject.toml was generated from pyproject.toml.m4, it was clear where the dependencies info came from. Now that it is not, it is relevant to explain the source of the info.
Out of scope of this PR: we may restore pyproject.toml.m4 but also keep the generated pyproject.toml in repo. Then we may remove the comment.
There was a problem hiding this comment.
When pyproject.toml was generated from pyproject.toml.m4, it was clear where the dependencies info came from. Now that it is not, it is relevant to explain the source of the info.
The dependencies are coming from the usage of these packages in sage-lib. If you want to specify the source, then you should mention the relevant subpackages.
Now that pyproject.toml is the source for depenendency info, the downstream file (in build pkgs) should refer it it; not the other way around
There was a problem hiding this comment.
More demands that are obviously outside of the scope of this PR.
| @@ -0,0 +1 @@ | |||
| cython | |||
There was a problem hiding this comment.
The reintroduction of these version requirement files containing no actual requirements (which however are present in the pyproject.toml file) seems pretty pointless. This also reverts the design of #36982 - which already went through the "disputed" process.
Just add all packages whose version is now specified in the pyproject toml in the list in bootstrap.
There was a problem hiding this comment.
seems pretty pointless.
No. This defines the map between SPKG names and Python distribution names.
There was a problem hiding this comment.
Just add all packages whose version is now specified in the pyproject toml in the list in
bootstrap.
No.
There was a problem hiding this comment.
... This also reverts the design of #36982 - which already went through the "disputed" process.
As I understand it, by the design of #36982:
src/pyproject.toml specifies version constraints of the dependencies of the distribution package sagemath-standard, independently from the sage-distribution.
This PR respects that, and also reintroduces build/pkgs/*/version_requirements.txt files to specify further version constraints of the sage packages for the sage-distribution.
@mkoeppe Am I right?
This PR reverts some of the changes made in #36982, because Matthias doesn't like the design of #36982 - which he expressed there of course already). Apart from these changes this PR looks good to me and I happily remove the "disputed" label once this is fixed. |
|
I removed the "disputed" label since we are doing the normal review process. When it is clear that there are still disagreements that cannot be resolved, we can add the "disputed" label. Under the "disputed" label, we are supposed to do voting instead. |
| @@ -1 +1 @@ | |||
| fpylll >=0.5.9 | |||
| fpylll | |||
There was a problem hiding this comment.
So the constraint ">= 0.5.9" is needed for sagemath-standard distribution package. For sage-the-distribution, there is no further constraint. Hence it is just "fpylll" here. Right?
Is there a sage package for which sage-the-distribution needs more constraint than sagemath-standard? It seems there is none, currently...
There was a problem hiding this comment.
As I said in https://groups.google.com/g/sage-devel/c/sVcJiJdUxRs/m/VMOu20agBgAJ, I don't plan to start maintaining two sets of constraints. (Setting the version constraint for the packages that are dependencies of sagelib in the pyproject.toml file instead of in the build/pkgs/*/version_requirements.txt file literally follows the design in Tobias's proof-of-concept PR #36982.)
There was a problem hiding this comment.
... Setting the version constraint for the packages that are dependencies of sagelib in the
pyproject.tomlfile instead of in thebuild/pkgs/*/version_requirements.txtfile literally follows the design in Tobias's proof-of-concept PR #36982.)
You mean src/pyproject.toml file? It is supposed to maintain version constrains for the dependencies of sagelib. This is what Tobias' PR did? This PR does not revert that. No?
Now this PR makes build/pkgs/*/version_requirements.txt maintain (additional) version constrains for the sage-distribution. No?
There was a problem hiding this comment.
Two sets of constraints (those in src/pyproject.toml and those in build/pkgs/*/version_requirements.txt ) serve different purposes. I think this is reasonable. Or I am misunderstanding things...
There was a problem hiding this comment.
You mean
src/pyproject.tomlfile?
Yes.
It is supposed to maintain version constrains for the dependencies of sagelib. This is what Tobias' PR did?
Consider a package A that is a build dependency of sagelib.
- Before Tobias's PR, the version constraints on A (for sagelib; for the modularized distributions; and for the Sage distribution in "configure --enable-system-site-packages" mode) were set in
build/pkgs/A/version_requirements.txt. - After Tobias's PR, the version constraints on A (for the same 3 purposes) are set in
src/pyproject.toml, andbuild/pkgs/A/version_requirements.txtis no longer a source file. - With the present PR, the file
build/pkgs/A/version_requirements.txtbecomes again a source file, but without actual version constraints. - In principle, the file could be used for additional version constraints on A, but I do not plan to use this mechanism. (Note that any version constraints there will take effect not only on the Sage distribution with "configure --enable-system-site-packages", but also for the modularized distributions.)
Now consider a package B that is not a build dependency of sagelib but a runtime dependency of sagelib.
- Tobias's PR made no changes regarding these packages.
- With the present PR, version constraints are applied in the same way as for package A.
There was a problem hiding this comment.
Using
version_requirements.txtfiles for this purpose seems wrong. These files were renamed to end onrequirementsso that standard tools pick them up. Its a misuse to use them for a sage-the-distro specific mapping.No, it's not a "misuse". It's the documented design.
Why is such a mapping needed in the first place? Just rename the SPKG to match the python distro name, and be done.
This does not work because the names sometimes clash with non-Python packages of the same name.
For which packages is this the case?
There was a problem hiding this comment.
non-python packages can be renamed if needed, this is not a problem
There was a problem hiding this comment.
one reason for name clashed between Python and non-Python packages is lack of a directory structure in build/pkgs/, everything is dumped into one flat namespace.
And no, don't tell me that a distribution XXX does this too, this is irrelevant, and it suffices to see that such an XXX is forced to deal with the name clashes like this by inventing package name prefixes, which is obviously just a poor man replacement of a proper directory structure.
There was a problem hiding this comment.
Obviously outside of the scope of this PR to do any of this.
|
@tobiasdiez Please specify what needs to be worked on when you add "needs work" label, so that other people can see what is going on with the PR. |
There are a few unresolved reviewer remarks above as comments directly against the changed files. That should be sufficient, or not? |
|
No. Unresolved remarks just need more discussion. |
Then please don't set it to positive review. I've said above that the reintroduction of committed version_requirements files is not okay with me; there is also no need to change this as part of a PR that aims to move metadata from setup.cfg to pyproject.toml. There is already a framework in place on how to specify infos about package dependencies in that file; just reuse exactly the same mechanism and create the version requirements files during bootstrap (and maybee improve it in the a follow up PR). |
|
I don't think this PR reverts the main of point of your PR #36982. The file This PR restores There was a need for additional version constraints (on cython, for example) for sage-the-distribution. I expected the Thus I am positive with this PR and added "positive review". In the discussions of you and Matthias, I see no items that you raised but Matthias didn't respond. It seemed that there is no item that Matthias will work on. It seems still so. Now that you added "needs work" again with no new items to discuss with Matthias, this PR is qualified for "disputed PR". After seeing Matthias' response to your "needs work" label, I will add "disputed" label to this PR. |
Per #36982 this map is defined in sage bootstrap, and is working well (e.g. the constraint for the python package cypari2 is correctly mapped to the sage package cypari - which by the way is the only package with a different slightly different name that I'm aware of [apart from some In either way, changing the way the build dependencies are handled is out-of-scope of this PR, since this information is already in pyproject.toml and no longer in |
Matthias makes the mapping explicit using
The scope of a PR is set by the PR author. Neither you nor me. You may request the author to make the scope explicit in the PR description. On the other hand, you may object to this PR on whatever ground. |
Wrong. |
|
@kwankyu explains it all correctly. |
sagemathgh-38577: Add various project URLs for PyPI <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> The changed file, `m4/pyproject_toml_metadata.m4`, controls the metadata of the modularized distributions. The same changes should be done to **sagemath-standard** and **sage- conf** after sagemath#37902, sagemath#36561 are merged. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38577 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
sagemathgh-38577: Add various project URLs for PyPI <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> The changed file, `m4/pyproject_toml_metadata.m4`, controls the metadata of the modularized distributions. The same changes should be done to **sagemath-standard** and **sage- conf** after sagemath#37902, sagemath#36561 are merged. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38577 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
sagemathgh-38577: Add various project URLs for PyPI <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> The changed file, `m4/pyproject_toml_metadata.m4`, controls the metadata of the modularized distributions. The same changes should be done to **sagemath-standard** and **sage- conf** after sagemath#37902, sagemath#36561 are merged. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38577 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
|
+1 from me. |
sagemathgh-38577: Add various project URLs for PyPI <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> The changed file, `m4/pyproject_toml_metadata.m4`, controls the metadata of the modularized distributions. The same changes should be done to **sagemath-standard** and **sage- conf** after sagemath#37902, sagemath#36561 are merged. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38577 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
sagemathgh-38577: Add various project URLs for PyPI <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> The changed file, `m4/pyproject_toml_metadata.m4`, controls the metadata of the modularized distributions. The same changes should be done to **sagemath-standard** and **sage- conf** after sagemath#37902, sagemath#36561 are merged. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38577 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
sagemathgh-38577: Add various project URLs for PyPI <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> The changed file, `m4/pyproject_toml_metadata.m4`, controls the metadata of the modularized distributions. The same changes should be done to **sagemath-standard** and **sage- conf** after sagemath#37902, sagemath#36561 are merged. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38577 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
|
A very fat -1 from me. All sorts of wrong reasons are given here to get this PR through, such as
etc. |
|
"to consider normal ways to deal with package name clashes" -- obviously outside of the scope of this PR. |
|
Closing in favor of: |
This PR recreates #36951 by @mkoeppe after it was reverted, see #37796.
Modernize the metadata.
This is a trivial "chore" PR. It updates Python metadata to the latest format. No controversies about the current format are known about in the Python community. In a typical open source project, someone in a Maintainer role would open a PR and then immediately merge it, or when receiving such a PR from the outside, quickly review and merge it (examples: my PRs pytest-dev/pytest-mock#410 (merged in within 1 day), pyodide/pyodide#4472, pytest-dev/pytest-xdist#1020,
sagemath/cypari2#158, fplll/fpylll#258, polymake/JuPyMake#2, cvxpy/cvxpy#2276, sagemath/cysignals#177).
Also fixes thepyproject.tomlbuild requirements of sagemath-standard, broken since 10.2, hence "critical".pkgs/sage-conf: Move metadata fromsetup.cfgtopyproject.toml#36561📝 Checklist
⌛ Dependencies
bootstrap,bootstrap-conda,m4/sage_spkg_collect.m4,sage-spkg-infothroughsage --package properties,sage-get-system-packages#37430Makefile,.ci/write-dockerfile.sh: Update forsrc/pyproject.tomlafter #36982 #37926