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

pkgs/sagemath-standard: Move metadata from setup.cfg to pyproject.toml #37902

Closed
wants to merge 9 commits into from

Conversation

saraedum
Copy link
Member

@saraedum saraedum commented Apr 29, 2024

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 the pyproject.toml build requirements of sagemath-standard, broken since 10.2, hence "critical".

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

⌛ Dependencies

@saraedum saraedum added s: needs review disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ labels Apr 29, 2024
@saraedum
Copy link
Member Author

I'm counting the votes from #36951:

👍 @jhpalmieri @mkoeppe @kwankyu @kiwifb

👎 @tornaria @tobiasdiez

(I am not the author of this PR, I didn't vote yet.)

Copy link

github-actions bot commented Apr 29, 2024

Documentation preview for this PR (built with commit 443b5bb; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@tornaria tornaria requested a review from orlitzky April 29, 2024 13:49
@tornaria
Copy link
Contributor

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:

  • I think moving to pyproject.toml is a good idea, as it's the modern way python packages work; however, it should be pyproject.toml and not pyproject.toml.m4.
  • You need to fix build dependencies. Right now you require a lot of dependencies just to create a sdist.
  • This conflicts with other disputed PRs and I believe the way to move forward is to come together and figure out a reasonable compromise between all the different positions

@mkoeppe

This comment was marked as abuse.

@orlitzky
Copy link
Contributor

I am -1 for now because I think #36982 is the better approach. But if that fails I would reconsider.

@NathanDunfield
Copy link
Contributor

+1: Moving from setup.cfg.m4 to pyproject.toml.m4 seems like a clear improvement. Vote summary is now:

👍 @jhpalmieri @mkoeppe @kwankyu @kiwifb @NathanDunfield
👎 @tornaria @tobiasdiez @orlitzky

@culler
Copy link
Contributor

culler commented Apr 30, 2024

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

@mkoeppe
Copy link
Contributor

mkoeppe commented May 17, 2024

I've reworked this on top of

This is no longer a trivial PR.

Also fixes the breakage noted in #37894 (comment)

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

LGTM.

@tobiasdiez
Copy link
Contributor

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).

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 6, 2024

I don't think this PR reverts the main of point of your PR #36982. The file src/pyproject.toml, with the dependencies info for the sage library (sagemath-standard), is still there.

This PR restores version_requirements.txt files, to establish the mapping between spkg names and pip package names, as Matthias explains. That info is not available from src/pyproject.toml. In this sense, this PR fixes a "bug" introduced by your PR #36982.

There was a need for additional version constraints (on cython, for example) for sage-the-distribution. I expected the version_requirements.txt file could be used for that purpose. But Matthias says that src/pyproject.toml serves the purpose, and provisioning a mechanism for additional set of version constraints is out of scope of this PR. I agree with that.

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.

@tobiasdiez
Copy link
Contributor

This PR restores version_requirements.txt files, to establish the mapping between spkg names and pip package names, as Matthias explains. That info is not available from src/pyproject.toml. In this sense, this PR fixes a "bug" introduced by your PR #36982.

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 - vs _)). Which bugs did you experience?

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 setup.cfg.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 6, 2024

This PR restores version_requirements.txt files, to establish the mapping between spkg names and pip package names, as Matthias explains. That info is not available from src/pyproject.toml. In this sense, this PR fixes a "bug" introduced by your PR #36982.

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 - vs _)).

Matthias makes the mapping explicit using version_requirements.txt files. Hardcoding the mapping in python code is the "bug" I am saying.

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 setup.cfg.

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 6, 2024

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 - vs _)).

Wrong.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 6, 2024

@kwankyu explains it all correctly.

@mkoeppe mkoeppe added s: needs review disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ and removed s: needs work labels Sep 6, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 6, 2024
    
<!-- ^ 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
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
    
<!-- ^ 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
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
    
<!-- ^ 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
@kwankyu
Copy link
Collaborator

kwankyu commented Sep 9, 2024

+1 from me.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 10, 2024
    
<!-- ^ 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
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 12, 2024
    
<!-- ^ 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
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 14, 2024
    
<!-- ^ 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
@dimpase
Copy link
Member

dimpase commented Sep 19, 2024

A very fat -1 from me. All sorts of wrong reasons are given here to get this PR through, such as

  • mkoeppe's "I am not going to maintain 2 sets of version constraints" - as if mkoeppe is the sole project owner
  • flat refusal to consider normal ways to deal with package name clashes

etc.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 25, 2024

"to consider normal ways to deal with package name clashes" -- obviously outside of the scope of this PR.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 26, 2024

@tobiasdiez
Copy link
Contributor

I've now cherry-picked the uncontroversial parts of this PR (or more specifically of #38714) to #38727. I think nobody objected that the metadata moved from the old setup.cfg to the modern pyproject.toml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: distribution disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ v: large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants