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

build/pkgs/python_build: Make it a standard wheel package #37300

Merged
merged 5 commits into from
Apr 27, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Feb 11, 2024

Vote: https://groups.google.com/g/sage-devel/c/MIU-xo9b7pc

Vote again: https://groups.google.com/g/sage-devel/c/EMeTJ6Hwgns (approved)

📝 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 Feb 12, 2024
@mkoeppe mkoeppe marked this pull request as ready for review February 15, 2024 08:46
@mkoeppe mkoeppe changed the title build/pkgs/python_build: Make standard build/pkgs/python_build: Make standard Feb 16, 2024
@mkoeppe mkoeppe changed the title build/pkgs/python_build: Make standard build/pkgs/python_build: Make it a standard wheel package Feb 18, 2024
@mkoeppe mkoeppe force-pushed the python_build_standard branch 2 times, most recently from 968051b to 7ceb349 Compare April 1, 2024 00:44
Copy link

github-actions bot commented Apr 10, 2024

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2024

The failures in CI Linux incremental are the unrelated #37786.

@mkoeppe mkoeppe requested a review from kiwifb April 14, 2024 21:14
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.

The addition has been approved and really nothing in here has any consequence on sagemath apart from the sudden availability of it in standard install.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2024

Thank you!

@mkoeppe mkoeppe requested a review from jhpalmieri April 15, 2024 20:11
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 15, 2024

Once again, someone is manipulating labels here.

@mkoeppe mkoeppe requested a review from jplab April 16, 2024 00:47
@jplab
Copy link

jplab commented Apr 16, 2024

@tobiasdiez: I am trying to understand the removal on the label "positive review" here. Could you help me out? Carefully checking the timestamp of every vote on sage-devel and checking the time of the removal of the "positive review", I can infer two things:

  • The negative votes were recorded after that the author expressed the approval.
  • Once the negative votes reached the threshold of "it shouldn't be a positive review yet", you removed the "positive review" tag.

Perhaps I am missing something: on the related sage-devel discussion, a vote has taken place. The author expressed the approval before others did put their vote in writing. Then, you changed the label 12 hours after, without explanation (which leaves me trying to figure out what happened here). Then, the discussion on sage-devel continues, and so do the votes, and as of right now, I count 5-2.

What I can see, is first that the current decision process on PRs requiring a vote has to be improved. This shall be reported on sage-devel, for a discussion with the community.

Second, the trace left on the development thread here is not what I, personally, qualify as constructive or transparent (as I am trying to understand what is going on, I am struggling!).

My suggestion, as a member of the Code of Conduct Committee, is that for transparency and constructiveness sake, I would suggest that changing labels should always be done with a justification (just like I was always used to on "trac" since 2010).

@saraedum
Copy link
Member

should always be done with a justification

Currently, @tobiasdiez is blocked from posting on @mkoeppe's PRs. I understand that that's why he could not explain his action here. I think it would have been helpful to propose (or at least explain) this label change on sage-devel in that case.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 16, 2024

I'm restoring the "positive review" label.

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

@dimpase it is my understanding that this is not a "disputed" PR. There is a regular vote on sage-devel about this PR. The vote has now been open for a week and a majority is in favor if I read it correctly. (Maybe the vote has been called a bit too quickly initially, btw.)

@saraedum saraedum added s: positive review and removed disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ labels Apr 16, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 18, 2024
… package

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
Vote: https://groups.google.com/g/sage-devel/c/MIU-xo9b7pc

Vote again: https://groups.google.com/g/sage-devel/c/EMeTJ6Hwgns
(approved)

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] 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
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#37300
Reported by: Matthias Köppe
Reviewer(s): François Bissey
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2024
… package

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
Vote: https://groups.google.com/g/sage-devel/c/MIU-xo9b7pc

Vote again: https://groups.google.com/g/sage-devel/c/EMeTJ6Hwgns
(approved)

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] 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
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#37300
Reported by: Matthias Köppe
Reviewer(s): François Bissey
@vbraun vbraun merged commit 6b9b03a into sagemath:develop Apr 27, 2024
21 of 41 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Apr 27, 2024
@mkoeppe mkoeppe deleted the python_build_standard branch April 27, 2024 17:52
vbraun pushed a commit to vbraun/sage that referenced this pull request May 2, 2024
    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

This is the next step of the long term effort to use the standard tools
of modern Python packaging in the build system of the Sage distribution,
enabled by making `python_build` a standard package. We add
`python_build` to our `PYTHON_TOOLCHAIN` and replace the use of `pip
wheel` by using `python -m build`.

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
- Resolves sagemath#34590.
- Also, we remove the unused helper function `sdh_prefix_args`.

<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] 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
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#37300

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#35618
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants