CI Linux: Replace use of pkill #36726

mkoeppe · 2023-11-15T19:42:42Z

Follow-up after:

.github/workflows/docker.yml: Interrupt the build before the 6 hour cancellation #36670

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

📝 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

kwankyu · 2023-11-16T11:06:59Z

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

dimpase · 2023-11-16T11:28:52Z

it's probably a short-lived bug in sid, and I say - send them a bug report, but don't bother with a dodgy workaround

mkoeppe · 2023-11-16T16:34:27Z

Actually, debian-sid was just an example, sorry for leading both of you on the wrong track. I'll update the ticket description

mkoeppe · 2023-11-16T16:44:03Z

Code in ci pipelines does not have to be pretty but should be clean enough to be maintainable by future maintainers...

Indeed. I've added a detailed comment.

kwankyu

Thanks. LGTM.

mkoeppe · 2023-11-17T01:13:24Z

Thank you!

dimpase · 2023-11-17T09:06:26Z

hmm, isn't "minimal" something we define? install packages with pkill in, done ...

mkoeppe · 2023-11-17T17:02:22Z

isn't "minimal" something we define?

It's something that we have defined, but not arbitrarily. It consists of the packages needed for bootstrapping and building the Sage distribution.

dimpase · 2023-11-17T23:00:14Z

on Debian pkill is in procps

so let's add procps to the list of CI Debian packages

mkoeppe · 2023-11-17T23:01:53Z

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.

dimpase · 2023-11-18T00:50:16Z

is pkill only used in CI? then why not just add it.

mkoeppe · 2023-11-18T00:51:54Z

I just explained it. If we add it, then the CI no longer tests what it should test.

dimpase · 2023-11-18T10:03:54Z

pkill is a part of testing infrastructure.

I just explained it. If we add it, then the CI no longer tests what it should

I just explained it. If we add it, then the CI no longer tests what it should test.

No, pkill is not part of Sage, it's a part of testing infrastructure. I don't see why you insist on testing infrastructure to be broken, doing dodgy things with /proc and what not instead. Negative review.

mkoeppe · 2023-11-18T14:50:26Z

Please, Dima, let's increase the level of discourse here.

mkoeppe · 2023-11-18T14:52:05Z

dodgy things with /proc and what not

/proc is the stable interface of the Linux kernel for precisely this information. I'm accessing it with standard Unix command line tools. If you have a concern about this, please be more specific.

kwankyu · 2023-11-21T02:19:10Z

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"

mkoeppe · 2024-01-12T22:49:10Z

@dimpase For your reference, here's the Code of Conduct from https://github.com/sagemath/sage/blob/develop/CODE_OF_CONDUCT.md [...]

Do you have questions about it? Otherwise, I'll go ahead and offer an interpretation with examples of violations, as previously promised above in #36726 (comment)

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.

…sions of the Sage distribution   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    ### 📝 Checklist     - [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   URL: sagemath#36977 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe

vbraun · 2024-01-23T23:58:38Z

Editor decision

Will merge since there is a PR ready. You can argue about minimal build images vs shortening one line in a shell script, but I just don't have strong feelings either way over something this inconsequential.

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      ### 📝 Checklist     - [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   URL: sagemath#36726 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Nathan Dunfield, William Stein

@mkoeppe

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), ...    ### 📝 Checklist     - [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   URL: sagemath#37452 Reported by: Matthias Köppe Reviewer(s): Giacomo Pope, Matthias Köppe

@roed314

…t PEP 725 (unbundled from sagemath#37446)       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  - [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    URL: sagemath#37482 Reported by: Matthias Köppe Reviewer(s):

@roed314

…t PEP 725 (unbundled from sagemath#37446)       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  - [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    URL: sagemath#37482 Reported by: Matthias Köppe Reviewer(s):

@roed314

…t PEP 725 (unbundled from sagemath#37446)       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  - [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    URL: sagemath#37482 Reported by: Matthias Köppe Reviewer(s):

mkoeppe added p: critical / 2 c: scripts labels Nov 15, 2023

mkoeppe added this to the sage-10.2 milestone Nov 15, 2023

mkoeppe self-assigned this Nov 15, 2023

mkoeppe added the s: needs review label Nov 15, 2023

mkoeppe requested a review from dimpase November 15, 2023 21:56

kwankyu approved these changes Nov 16, 2023

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Nov 16, 2023

dimpase added s: needs review s: needs work and removed s: positive review s: needs review labels Nov 18, 2023

mkoeppe added s: needs review and removed s: needs work labels Nov 18, 2023

mkoeppe added the s: positive review label Jan 7, 2024

tobiasdiez removed the s: positive review label Jan 7, 2024

mkoeppe mentioned this pull request Jan 9, 2024

tox -e update_docker_platforms #36954

Merged

5 tasks

mkoeppe added the s: positive review label Jan 10, 2024

tobiasdiez removed the s: positive review label Jan 10, 2024

vbraun added the s: positive review label Jan 29, 2024

vbraun merged commit e7565a6 into sagemath:develop Feb 2, 2024
77 of 83 checks passed

github-actions bot removed the s: positive review label Feb 2, 2024

tornaria mentioned this pull request Feb 19, 2024

Readd init files #36753

Open

5 tasks

mkoeppe deleted the ci_self_destruct_without_pkill branch February 20, 2024 01:44

mkoeppe added this to the sage-10.3 milestone Mar 7, 2024

mkoeppe mentioned this pull request Mar 20, 2024

CI: Update Linux platforms / runners, remove ubuntu-trusty, opensuse-{15.3,15.4} #37351

Merged

5 tasks

This was referenced Apr 17, 2024

Revert PR with disputed dependencies #37796

Merged

/usr/bin/time should be a pre-req #37832

Closed

mkoeppe mentioned this pull request Apr 30, 2024

symengine and symengine_py 0.11 #36677

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI Linux: Replace use of pkill #36726

CI Linux: Replace use of pkill #36726

mkoeppe commented Nov 15, 2023 •

edited

Loading

kwankyu commented Nov 16, 2023 •

edited

Loading

dimpase commented Nov 16, 2023

mkoeppe commented Nov 16, 2023

mkoeppe commented Nov 16, 2023

kwankyu left a comment

mkoeppe commented Nov 17, 2023

dimpase commented Nov 17, 2023

mkoeppe commented Nov 17, 2023

dimpase commented Nov 17, 2023

mkoeppe commented Nov 17, 2023

dimpase commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

dimpase commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

kwankyu commented Nov 21, 2023 •

edited

Loading

mkoeppe commented Jan 12, 2024

vbraun commented Jan 23, 2024

CI Linux: Replace use of pkill #36726

CI Linux: Replace use of pkill #36726

Conversation

mkoeppe commented Nov 15, 2023 • edited Loading

📝 Checklist

⌛ Dependencies

kwankyu commented Nov 16, 2023 • edited Loading

dimpase commented Nov 16, 2023

mkoeppe commented Nov 16, 2023

mkoeppe commented Nov 16, 2023

kwankyu left a comment

Choose a reason for hiding this comment

mkoeppe commented Nov 17, 2023

dimpase commented Nov 17, 2023

mkoeppe commented Nov 17, 2023

dimpase commented Nov 17, 2023

mkoeppe commented Nov 17, 2023

dimpase commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

dimpase commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

mkoeppe commented Nov 18, 2023

kwankyu commented Nov 21, 2023 • edited Loading

mkoeppe commented Jan 12, 2024

vbraun commented Jan 23, 2024

Editor decision

mkoeppe commented Nov 15, 2023 •

edited

Loading

kwankyu commented Nov 16, 2023 •

edited

Loading

kwankyu commented Nov 21, 2023 •

edited

Loading