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/sage-conf: Move metadata from setup.cfg to pyproject.toml #36561

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Oct 28, 2023

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

📝 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

@mkoeppe mkoeppe self-assigned this Oct 28, 2023
@mkoeppe mkoeppe changed the title pkgs/sage-conf: Move metadata from setup.cfg to pyproject.toml pkgs/sage-conf: Move metadata from setup.cfg to pyproject.toml Oct 28, 2023
@kiwifb
Copy link
Member

kiwifb commented Oct 29, 2023

There should be links for sage_conf_conda and sage_conf_pypi or do they not show up in github?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 29, 2023

These links are already in the repo and there's no change to them on this PR.

@kiwifb
Copy link
Member

kiwifb commented Oct 29, 2023

Scrap that, I thought the file was brand new. It is already linked. Too early on Monday for me to review properly.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 29, 2023

Note that there was already pyproject.toml for the build-system information; here we're just adding more sections to the file.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 29, 2023

Happy Monday!

@tobiasdiez
Copy link
Contributor

Please make sure that this change is compatible with the usage of pyproject.toml in #36489 (I think that PR uses the default config that you overwrite here).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 29, 2023

Sure, there's no reason for that implementation of sage-conf to be incompatible with this pyproject.toml.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 2, 2023

Let's get this in please

@tobiasdiez
Copy link
Contributor

Sure, there's no reason for that implementation of sage-conf to be incompatible with this pyproject.toml.

I've tried it with #36489 and its not working as there is no _sage_conf folder anymore. I think its better to stick with the defaults (i.e. either a src or sage_conf folder containing the sources).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 2, 2023

That #36489 is not ready cannot be blamed on this PR.

Copy link
Member

@kiwifb kiwifb left a comment

Choose a reason for hiding this comment

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

LGTM, I do not have any unexpected issue with it in sage-on-gentoo.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 2, 2023

Thank you!

@tobiasdiez
Copy link
Contributor

As I've said before, these lines

packages = ["_sage_conf"]
py-modules = ["sage_conf"]

are not okay for me.

(I think its also not polite to cherry pick commits from a PR and to then introduce changes that lead to conflicts with that PR)

@mkoeppe

This comment was marked as abuse.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 3, 2024

9 months later, this PR is still open.

The current tally:

Please vote!

Copy link
Contributor

@culler culler left a comment

Choose a reason for hiding this comment

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

This PR seems to replace a pyproject.toml file by an m4 template that can be used to
automatically update version information when generating the pyproject.toml file for a new version. I don't see anything particularly controversial about that. There are surely alternatives to using m4, but Sage is already using m4, so this choice does not make a significant difference.

@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 4, 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
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
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 p: critical / 2 s: needs review v: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.