-
-
Notifications
You must be signed in to change notification settings - Fork 655
CI Linux: Replace use of pkill #36726
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
CI Linux: Replace use of pkill #36726
Conversation
The code is getting arcane. This is to support debian-sid. I am very ignorant of the Linux eco-system, and my naive question is: Do we need to support debian-sid? By a quick search, sid is said to be very unstable version of debian and is never released for end users. So it seems to make little sense that we should write such arcane code to support it... Code in ci pipelines does not have to be pretty but should be clean enough to be maintainable by future maintainers... |
it's probably a short-lived bug in |
Actually, debian-sid was just an example, sorry for leading both of you on the wrong track. I'll update the ticket description |
Indeed. I've added a detailed comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. LGTM.
Thank you! |
hmm, isn't "minimal" something we define? install packages with pkill in, done ... |
It's something that we have defined, but not arbitrarily. It consists of the packages needed for bootstrapping and building the Sage distribution. |
on Debian pkill is in procps so let's add procps to the list of CI Debian packages |
The major point of the "minimal" package configuration is to test that the documented minimal list of build requirements works. Adding random stuff to it weakens this purpose. |
is pkill only used in CI? then why not just add it. |
I just explained it. If we add it, then the CI no longer tests what it should test. |
No, |
Please, Dima, let's increase the level of discourse here. |
|
Do we have a mechanism to resolve the dead-lock state of a PR like this one? I don't think so. As we have several such PRs (where labelling wars are going on) by now, the lack of the mechanism seems seriously hinder the progress of our project. I know that Matthias does not like poll as the mechanism. So I suggest a mild alternative (automatic poll) to it. First let me be clear that I designed the alternative mechanism to be advantageous to the author. (1) A PR is in dead-lock state when there are "Approving" reviews and "Requesting changes" (disapproving) reviews that appear on the "Reviewers" section on the upper right side, and the author is not accepting the work items or the reasons of the disapproving reviews. If a PR is in dead-lock state, we put both "needs work" (or "needs info") and "needs review" label to the PR. (2) If a PR is in dead-lock state, other people may come and give their own "Approving" or "Disapproving" reviews using the GitHub review system. (3) If a week has passed in dead-lock state with more "Approving" reviews than "Disapproving" reviews, the author attains the right to put "positive review" label to the PR, and the dead-lock state is resolved. If both of you agree, then I may post this proposal to sage-devel to get community-wide approval. Edit: "negative review" was replaced with "disapproving review" |
I think I have to caution that using the phrase "negative review" in drafting such rules for PRs is already highly problematic. The search https://github.com/sagemath/sage/issues?q=sort%3Aupdated-desc+%22negative+review%22 shows that this phrase has not been used in Sage development discussions since around 2010, until its reintroduction this year. This phrase basically denies that:
Instead, this phrase suggests that decisions can be taken just based on individuals being "for" or "against" something; that opinions matter but reasons do not. Designing conflict resolution based on counting "positive reviews" against "negative reviews" reinforces this. |
As I see it, the cause of labelling war is "political" instead of "technical". If the issue is "technical", there is a better and right solution that we can converge into by more discussion. When the issue is "political", there are more than one good solutions (or bad solutions depending on your point of view) and it's a matter of choice to choose one rather than the other other. Then how do we know that an issue is "political"? Practically we should admit that the issue is "political" when we go on labelling war instead of doing discussion. |
We may use "Disapproving review" instead of "negative review" if you prefer. By the way, I am abusing "Requesting changes" as a means to express "Disapproving" review as there is no other alternative way. |
Besides, you never explained how getting My proposal to fix this issue is as follows: diff --git a/build/pkgs/_prereq/distros/alpine.txt b/build/pkgs/_prereq/distros/alpine.txt
index 159c452cf30..350cc8e2c26 100644
--- a/build/pkgs/_prereq/distros/alpine.txt
+++ b/build/pkgs/_prereq/distros/alpine.txt
@@ -9,3 +9,4 @@ gcc
g++
ca-certificates
coreutils
+procops
diff --git a/build/pkgs/_prereq/distros/arch.txt b/build/pkgs/_prereq/distros/arch.txt
index 9b9bf09e7e2..f2631868f4e 100644
--- a/build/pkgs/_prereq/distros/arch.txt
+++ b/build/pkgs/_prereq/distros/arch.txt
@@ -8,3 +8,4 @@ bc
gcc
# Needed for 4ti2:
which
+procps
diff --git a/build/pkgs/_prereq/distros/conda.txt b/build/pkgs/_prereq/distros/conda.txt
index d76388ce7bb..26fc374139d 100644
--- a/build/pkgs/_prereq/distros/conda.txt
+++ b/build/pkgs/_prereq/distros/conda.txt
@@ -5,3 +5,4 @@ perl
python
tar
bc
+procps
diff --git a/build/pkgs/_prereq/distros/debian.txt b/build/pkgs/_prereq/distros/debian.txt
index 50be6ac7e0b..e322a08ba2e 100644
--- a/build/pkgs/_prereq/distros/debian.txt
+++ b/build/pkgs/_prereq/distros/debian.txt
@@ -21,6 +21,7 @@ python3
tar
bc
gcc
+procps
# On debian buster, need C++ even to survive 'configure'. Otherwise:
# checking how to run the C++ preprocessor... /lib/cpp
# configure: error: in `/sage':
diff --git a/build/pkgs/_prereq/distros/fedora.txt b/build/pkgs/_prereq/distros/fedora.txt
index b35d7f64faf..250578c31ef 100644
--- a/build/pkgs/_prereq/distros/fedora.txt
+++ b/build/pkgs/_prereq/distros/fedora.txt
@@ -28,6 +28,7 @@ gcc
gcc-c++
# Needed according to embray at https://github.com/sagemath/sage/issues/26964:
# The need for which comes [...] from MPIR's configure script
+procps
findutils
which
diffutils
diff --git a/build/pkgs/_prereq/distros/gentoo.txt b/build/pkgs/_prereq/distros/gentoo.txt
index 1e26c46cacc..b624ae66e5d 100644
--- a/build/pkgs/_prereq/distros/gentoo.txt
+++ b/build/pkgs/_prereq/distros/gentoo.txt
@@ -18,3 +18,4 @@ dev-libs/libxml2
sys-apps/findutils
sys-apps/which
sys-apps/diffutils
+sys-process/procps
diff --git a/build/pkgs/_prereq/distros/opensuse.txt b/build/pkgs/_prereq/distros/opensuse.txt
index 6f7a11fea47..718028b2557 100644
--- a/build/pkgs/_prereq/distros/opensuse.txt
+++ b/build/pkgs/_prereq/distros/opensuse.txt
@@ -27,3 +27,4 @@ gzip
# Trac #32368
findutils
diffutils
+procps
diff --git a/build/pkgs/_prereq/distros/slackware.txt b/build/pkgs/_prereq/distros/slackware.txt
index 4c957e45264..c093cd0aeb7 100644
--- a/build/pkgs/_prereq/distros/slackware.txt
+++ b/build/pkgs/_prereq/distros/slackware.txt
@@ -16,3 +16,4 @@ flex
ca-certificates
libxml2
cyrus-sasl
+procps
diff --git a/build/pkgs/_prereq/distros/void.txt b/build/pkgs/_prereq/distros/void.txt
index 552b5a415f2..257fdd94161 100644
--- a/build/pkgs/_prereq/distros/void.txt
+++ b/build/pkgs/_prereq/distros/void.txt
@@ -8,3 +8,4 @@ perl
python3
tar
which
+procps |
Only Linux matters here because this line is executed in a Linux Docker container. And no, for Linux, That it's present on all the systems you looked at is explained by these systems having an installation of many packages. |
yes, so why not just add another package? |
Yes, I understood that this is what you propose when you said it in #36726 (comment) By the way, the package name, |
I explained it in #36726 (comment), #36726 (comment), #36726 (comment), but I'll elaborate:
|
As the misconduct has continued in https://groups.google.com/g/sage-devel/c/RdSImkzRxJI/m/CMKBJ6u6BAAJ, https://groups.google.com/g/sage-devel/c/XON6NTJa33o/m/_AmwQ6s6BAAJ, #32532 (comment), I'll proceed. |
sagemathgh-36977: `sage --package metrics`: New tool to assist discussions of the Sage distribution <!-- ^^^^^ 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 --> Discussions of the complexity of the Sage distribution pop up unexpectedly, as seen in sagemath#36982 (comment), sagemath#36982 (comment), sagemath#36982 (comment), sagemath#36982 (comment), sagemath#36982 (comment), sagemath#36982 (comment), sagemath#36982 (comment) sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment), sagemath#36726 (comment) sagemath#36777 (comment), sagemath#36777 (comment), sagemath#36777 (comment), sagemath#36777 (comment), sagemath#36777 (comment), sagemath#36777 (comment) To help put such discussions on a solid factual basis, we introduce the command `sage --package metrics`. ``` $ ./sage -package metrics :standard: has_file_distros_arch_txt=131 has_file_distros_conda_txt=216 has_file_distros_debian_txt=125 has_file_distros_fedora_txt=138 has_file_distros_gentoo_txt=181 has_file_distros_homebrew_txt=61 has_file_distros_macports_txt=129 has_file_distros_nix_txt=51 has_file_distros_opensuse_txt=146 has_file_distros_slackware_txt=25 has_file_distros_void_txt=184 has_file_patches=35 has_file_spkg_check=59 has_file_spkg_configure_m4=222 has_file_spkg_install=198 has_tarball_upstream_url=231 line_count_file_patches=22561 line_count_file_spkg_check=402 line_count_file_spkg_configure_m4=2792 line_count_file_spkg_install=2960 packages=272 type_standard=272 ``` Use `PATH=build/bin:$PATH SAGE_ROOT=some-other-worktree build/bin/sage- package metrics :standard:` to obtain the metrics of another version of Sage in some other worktree. We add computation and before/after comparison of the metrics to the CI Linux Incremental workflow. As an illustration, we change one Python package from "normal" to "wheel", removing an `spkg-install.in` file in the process. See https:// github.com/sagemath/sage/actions/runs/7342841283/job/19992606617?pr=3697 7#step:6:12 More metrics can be added after - sagemath#36740 <!-- 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. - [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 - 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#36977 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
Follow-up after: - sagemath#36670 This is working well, for example see https://github.com/mkoeppe/sage/ac tions/runs/6874334523/job/18695809308#step:11:5345, making it possible to obtain the image of the hanging build - https://github.com/mkoeppe/sa ge/actions/runs/6874334523/job/18695809308#step:15:25 However, `pkill` is not present on many of our `-minimal` configurations (which test our documented minimal build prerequisites). For example `debian-sid-minimal`, as seen in https://github.com/mkoeppe/sage/actions /runs/6874334523/job/18695811066#step:11:2262, rendering the mechanism ineffective there. (Neither are `ps` and `killall`.) Examples: - `docker run -it ghcr.io/sagemath/sage/sage-debian-bookworm-minimal- with-system-packages:dev bash -c "command -v pkill" ` - `docker run -it ghcr.io/sagemath/sage/sage-fedora-38-minimal-with- system-packages:dev bash -c "command -v pkill"` - whereas `docker run -it ghcr.io/sagemath/sage/sage-fedora-38-standard- with-system-packages:dev bash -c "command -v pkill" ` --> `/usr/bin/pkill`. We replace `pkill` by more primitive means. Test run (with timeout set to ~11 minutes for testing purposes): https:/ /github.com/mkoeppe/sage/actions/runs/6883232154/job/18723359505#step:15 :25 <!-- ^^^^^ 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 --> <!-- 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. - [x] 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#36726 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Nathan Dunfield, William Stein
Follow-up after: - sagemath#36670 This is working well, for example see https://github.com/mkoeppe/sage/ac tions/runs/6874334523/job/18695809308#step:11:5345, making it possible to obtain the image of the hanging build - https://github.com/mkoeppe/sa ge/actions/runs/6874334523/job/18695809308#step:15:25 However, `pkill` is not present on many of our `-minimal` configurations (which test our documented minimal build prerequisites). For example `debian-sid-minimal`, as seen in https://github.com/mkoeppe/sage/actions /runs/6874334523/job/18695811066#step:11:2262, rendering the mechanism ineffective there. (Neither are `ps` and `killall`.) Examples: - `docker run -it ghcr.io/sagemath/sage/sage-debian-bookworm-minimal- with-system-packages:dev bash -c "command -v pkill" ` - `docker run -it ghcr.io/sagemath/sage/sage-fedora-38-minimal-with- system-packages:dev bash -c "command -v pkill"` - whereas `docker run -it ghcr.io/sagemath/sage/sage-fedora-38-standard- with-system-packages:dev bash -c "command -v pkill" ` --> `/usr/bin/pkill`. We replace `pkill` by more primitive means. Test run (with timeout set to ~11 minutes for testing purposes): https:/ /github.com/mkoeppe/sage/actions/runs/6883232154/job/18723359505#step:15 :25 <!-- ^^^^^ 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 --> <!-- 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. - [x] 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#36726 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Nathan Dunfield, William Stein
Follow-up after: - sagemath#36670 This is working well, for example see https://github.com/mkoeppe/sage/ac tions/runs/6874334523/job/18695809308#step:11:5345, making it possible to obtain the image of the hanging build - https://github.com/mkoeppe/sa ge/actions/runs/6874334523/job/18695809308#step:15:25 However, `pkill` is not present on many of our `-minimal` configurations (which test our documented minimal build prerequisites). For example `debian-sid-minimal`, as seen in https://github.com/mkoeppe/sage/actions /runs/6874334523/job/18695811066#step:11:2262, rendering the mechanism ineffective there. (Neither are `ps` and `killall`.) Examples: - `docker run -it ghcr.io/sagemath/sage/sage-debian-bookworm-minimal- with-system-packages:dev bash -c "command -v pkill" ` - `docker run -it ghcr.io/sagemath/sage/sage-fedora-38-minimal-with- system-packages:dev bash -c "command -v pkill"` - whereas `docker run -it ghcr.io/sagemath/sage/sage-fedora-38-standard- with-system-packages:dev bash -c "command -v pkill" ` --> `/usr/bin/pkill`. We replace `pkill` by more primitive means. Test run (with timeout set to ~11 minutes for testing purposes): https:/ /github.com/mkoeppe/sage/actions/runs/6883232154/job/18723359505#step:15 :25 <!-- ^^^^^ 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 --> <!-- 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. - [x] 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#36726 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Nathan Dunfield, William Stein
sagemathgh-36726: CI Linux: Replace use of pkill Follow-up after: - sagemath#36670 This is working well, for example see https://github.com/mkoeppe/sage/ac tions/runs/6874334523/job/18695809308#step:11:5345, making it possible to obtain the image of the hanging build - https://github.com/mkoeppe/sa ge/actions/runs/6874334523/job/18695809308#step:15:25 However, `pkill` is not present on many of our `-minimal` configurations (which test our documented minimal build prerequisites). For example `debian-sid-minimal`, as seen in https://github.com/mkoeppe/sage/actions /runs/6874334523/job/18695811066#step:11:2262, rendering the mechanism ineffective there. (Neither are `ps` and `killall`.) Examples: - `docker run -it ghcr.io/sagemath/sage/sage-debian-bookworm-minimal- with-system-packages:dev bash -c "command -v pkill" ` - `docker run -it ghcr.io/sagemath/sage/sage-fedora-38-minimal-with- system-packages:dev bash -c "command -v pkill"` - whereas `docker run -it ghcr.io/sagemath/sage/sage-fedora-38-standard- with-system-packages:dev bash -c "command -v pkill" ` --> `/usr/bin/pkill`. We replace `pkill` by more primitive means. Test run (with timeout set to ~11 minutes for testing purposes): https:/ /github.com/mkoeppe/sage/actions/runs/6883232154/job/18723359505#step:15 :25 <!-- ^^^^^ 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 --> <!-- 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. - [x] 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#36726 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Nathan Dunfield, William Stein
sagemathgh-37452: Add config for ruff (sagemath#36503, unbundled) <!-- ^^^^^ 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 --> Author: @mkoeppe, based on @tobiasdiez's config in sagemath#36503. Adding a configuration for the ruff linter as proposed in sagemath#36503 is timely and uncontroversial. However, sagemath#36503 bundled this gratuitously with the controversial design of creating a `pyproject.toml` file in SAGE_ROOT. `pyproject.toml` -- by definition in PEP 518, PEP 621 -- marks a directory as a Python project, i.e., the source directory of a Python distribution package (sagemath#36503 (comment)). Generalizing the use of `pyproject.toml` to "[non-package projects](https://peps.python.org/pep-0735/#motivation)" is still subject to discussion in the Python community in [PEP 735](https://peps.python.org/pep-0735/) (Nov 2023). **The scope of PRs should be chosen to minimize friction, not to maximize friction.** sagemath#36726 (comment) Here we remove the artificial friction from the gratuitous bundling, by configuring ruff in `ruff.toml` instead. Configuration in ruff.toml and pyproject.toml has equal validity / standing per [ruff documentation](https://docs.astral.sh/ruff/configuration/#config-file- discovery). And this is the location of most of our other linter configuration files. Reference on previous common denominator PRs: sagemath#36666 (comment), sagemath#36666 (comment), sagemath#36572 (comment), sagemath#36960 (comment), sagemath#36960 (comment), ... <!-- 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. - [x] 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#37452 Reported by: Matthias Köppe Reviewer(s): Giacomo Pope, Matthias Köppe
sagemathgh-37482: src/pyproject.toml: Add 'external' section per draft 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):
sagemathgh-37482: src/pyproject.toml: Add 'external' section per draft 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):
sagemathgh-37482: src/pyproject.toml: Add 'external' section per draft 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):
Follow-up after:
This is working well, for example see https://github.com/mkoeppe/sage/actions/runs/6874334523/job/18695809308#step:11:5345, making it possible to obtain the image of the hanging build - https://github.com/mkoeppe/sage/actions/runs/6874334523/job/18695809308#step:15:25
However,
pkill
is not present on many of our-minimal
configurations (which test our documented minimal build prerequisites). For exampledebian-sid-minimal
, as seen in https://github.com/mkoeppe/sage/actions/runs/6874334523/job/18695811066#step:11:2262, rendering the mechanism ineffective there. (Neither areps
andkillall
.)Examples:
docker run -it ghcr.io/sagemath/sage/sage-debian-bookworm-minimal-with-system-packages:dev bash -c "command -v pkill"
docker run -it ghcr.io/sagemath/sage/sage-fedora-38-minimal-with-system-packages:dev bash -c "command -v pkill"
docker run -it ghcr.io/sagemath/sage/sage-fedora-38-standard-with-system-packages:dev bash -c "command -v pkill"
-->/usr/bin/pkill
.We replace
pkill
by more primitive means.Test run (with timeout set to ~11 minutes for testing purposes): https://github.com/mkoeppe/sage/actions/runs/6883232154/job/18723359505#step:15:25
📝 Checklist
⌛ Dependencies