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

src/pyproject.toml: Add 'external' section per draft PEP 725 (unbundled from #37446) #37482

Merged

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Feb 26, 2024

This is aspirational decoration for future use by skeleton generators by distributions such as conda, sage-the-distribution, pyodide.

Split out from the disputed #37446, where it is bundled with a number of other changes, including: creating a pyproject.toml file in SAGE_ROOT, hardcoding versions of Python packages instead of using the existing sage_bootstrap infrastructure, etc. @roed314 @vbraun

The scope of PRs should be chosen to minimize friction, not to maximize friction.
#36726 (comment)

Author: @mkoeppe, based on @tobiasdiez's #37446.

📝 Checklist

  • The title is concise and informative.
  • 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 changed the title src/pyproject.toml.m4: Add 'external' section (per draft PEP 725) src/pyproject.toml.m4: Add 'external' section per draft PEP 725 (unbundled from #37446) Feb 26, 2024
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 27, 2024

For the record (and summary of my DMs with @roed314 on Zulip), there's absolutely nothing wrong with #37476 cherry-picking one of my commits from #37351 and giving it a positive review.

@mkoeppe mkoeppe force-pushed the pyproject_external_sagemath_standard branch from 1e472cd to e5f7ec2 Compare March 14, 2024 07:28
@mkoeppe mkoeppe requested review from saraedum and removed request for tobiasdiez March 20, 2024 19:42
@mkoeppe mkoeppe self-assigned this Mar 20, 2024
@mkoeppe mkoeppe force-pushed the pyproject_external_sagemath_standard branch 2 times, most recently from 3fa06fb to c2a9840 Compare April 1, 2024 00:47
Copy link

github-actions bot commented Apr 1, 2024

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

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 17, 2024

This is at this point just a record of external dependencies. LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 18, 2024

@roed314 @saraedum @jhpalmieri @VivianePons Someone here is manipulating labels again.

@roed314
Copy link
Contributor

roed314 commented Apr 18, 2024

Thanks for the pointer. I'll discuss a response with the committee.

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2024
… 'external' section according to draft PEP 725

    
<!-- ^ 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". -->

As done in sagemath#37482 (unbundled from sagemath#37446) for **sagemath-standard**.

- This information will be used in the skeleton generator in
pyodide/pyodide#4438

### 📝 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 accordingly.

### ⌛ 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#37486
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@roed314
Copy link
Contributor

roed314 commented Apr 20, 2024

@tobiasdiez has objected to this PR, but cannot comment here. See the here for his reasons.

For now, I've set this PR to disputed, and am recording a -1 from Tobias.

@roed314 roed314 added disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ and removed pending labels Apr 20, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 25, 2024
… 'external' section according to draft PEP 725

    
<!-- ^ 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". -->

As done in sagemath#37482 (unbundled from sagemath#37446) for **sagemath-standard**.

- This information will be used in the skeleton generator in
pyodide/pyodide#4438

### 📝 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 accordingly.

### ⌛ 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#37486
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@mkoeppe mkoeppe force-pushed the pyproject_external_sagemath_standard branch from c2a9840 to c99164a Compare May 8, 2024 01:39
@mkoeppe mkoeppe changed the title src/pyproject.toml.m4: Add 'external' section per draft PEP 725 (unbundled from #37446) src/pyproject.toml: Add 'external' section per draft PEP 725 (unbundled from #37446) May 8, 2024
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 8, 2024

Resolved merge conflict caused by #36982.

@mkoeppe mkoeppe force-pushed the pyproject_external_sagemath_standard branch from c99164a to 07cbd65 Compare May 25, 2024 18:20
@mkoeppe mkoeppe force-pushed the pyproject_external_sagemath_standard branch from 07cbd65 to 505398a Compare June 1, 2024 23:42
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2024

@roed314 @saredum @jipilab The "disputed" should be removed here; there's no review or anything here.

@saraedum
Copy link
Member

saraedum commented Jun 6, 2024

@roed314 @saredum @jipilab The "disputed" should be removed here; there's no review or anything here.

In my personal opinion, it's @tobiasdiez decision whether to remove the label. As for the missing review, I believe that you blocked him here on GitHub so he cannot go through the normal review process first.

Our rules are imo unclear about what is supposed to happen in this case. The rules ask to set to "Needs Work" and leave a comment first. Since that's not an option I guess it's acceptable to skip to "disputed". If that's not how the rules should be interpreted, then maybe an amended version of the rules should be called for a vote on sage-devel to cover this case.

@mkoeppe mkoeppe force-pushed the pyproject_external_sagemath_standard branch from 505398a to 820c753 Compare June 13, 2024 06:03
@mkoeppe mkoeppe force-pushed the pyproject_external_sagemath_standard branch from 820c753 to 477d491 Compare June 23, 2024 00:51
@mkoeppe mkoeppe force-pushed the pyproject_external_sagemath_standard branch from 477d491 to ddfd7c0 Compare July 25, 2024 10:52
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 27, 2024

Let's just go through the "disputed PRs" procedure. The PR has carried the "disputed" label since Apr 20.
I'm counting:

vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 31, 2024
…t PEP 725 (unbundled from sagemath#37446)

    
<!-- ^ 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". -->

This is aspirational decoration for future use by skeleton generators by
distributions such as conda, sage-the-distribution, pyodide.

Split out from the disputed sagemath#37446, where it is bundled with a number of
other changes, including: creating a `pyproject.toml` file in
`SAGE_ROOT`, hardcoding versions of Python packages instead of using the
existing `sage_bootstrap` infrastructure, etc. @roed314 @vbraun

**The scope of PRs should be chosen to minimize friction, not to
maximize friction.**
sagemath#36726 (comment)

Author: @mkoeppe, based on @tobiasdiez's sagemath#37446.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [X] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ 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#37482
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 2, 2024
…t PEP 725 (unbundled from sagemath#37446)

    
<!-- ^ 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". -->

This is aspirational decoration for future use by skeleton generators by
distributions such as conda, sage-the-distribution, pyodide.

Split out from the disputed sagemath#37446, where it is bundled with a number of
other changes, including: creating a `pyproject.toml` file in
`SAGE_ROOT`, hardcoding versions of Python packages instead of using the
existing `sage_bootstrap` infrastructure, etc. @roed314 @vbraun

**The scope of PRs should be chosen to minimize friction, not to
maximize friction.**
sagemath#36726 (comment)

Author: @mkoeppe, based on @tobiasdiez's sagemath#37446.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [X] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ 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#37482
Reported by: Matthias Köppe
Reviewer(s):
@vbraun vbraun merged commit 8925453 into sagemath:develop Aug 3, 2024
18 of 19 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 3, 2024
@mkoeppe mkoeppe deleted the pyproject_external_sagemath_standard branch August 3, 2024 17:21
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: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants