GH Actions: Run incremental portability tests on "c: packages: standard" PRs #35380
Conversation
mkoeppe
force-pushed
the
ci_portability_on_label
branch
from
e5bf64b
to
5e865a8
Compare
There was a problem hiding this comment.
Code-wise it looks good to me.
I don't know which systems are good test candidates. For the old systems, you want to hit somewhere between "reasonably old to span a distance to the newer systems" vs "too old and makes only problems during package upgrades". Some of the systems seem a bit too old for my taste to still invest resources in continuously supporting them (e.g. when the PR fails on them but not the more modern versions). Maybe other devs have more ideas what systems should be considered the minimum supported version (with the older portability tests being nice bonuses if they pass). @dimpase ?
|
Indeed, some systems there are waytoo old, and people who run them are at risk of being hacked. We should not support such systems. I don't see e.g. why fedora-31 is there (it's EOL since 2020-11-24) `https://docs.fedoraproject.org/en-US/releases/eol/ |
|
I also don't see a point in Actually, this applies to the whole |
The |
Discussions of what systems to support are best placed in #32074. |
Yes, this is a good observation, and is part of the considerations that I use. |
Yes, all fedora releases are very short-lived; there's no LTS. I'm using old fedora as a proxy for RHEL releases. RHEL 8 is based on Fedora 28 (which we no longer test, see #32074). |
No, because we are running this workflow to test package installation. If we wanted to use also some "standard" configurations, we would have to determine which packages in |
Yes, there's something that needs to be improved. The 32bit target is building too many packages. The I'll investigate. |
No, looking right. This is testing the Rare event, will only happen with upgrades of the |
|
Restarted fedora-37 at https://github.com/mkoeppe/sage/actions/runs/4922782397/jobs/8816740288 |
|
This time fedora-37 went through normally. So nothing wrong with it. |
|
So let's get this in please. Refinements along the lines of #35380 (comment) can be made later if desired |
|
What would be the work item here? |
This comment was marked as abuse.
This comment was marked as abuse.
Dima, this is highly inappropriate. I have referred this message, in which you appear to endorse retaliatory behavior on PRs, to sage-abuse. |
Addressing the reviewer comments?!
I think you misunderstood Dima. In my view, he was only factually describing the situation. We have a PR here that is got reviewer comments where the creator doesn't agree with these comments. |
Are you referring to the demand that I "propose my own compromise" (a compromise with what or with who?) or something else? Dima had a question re fedora; I have answered it. |
mkoeppe
force-pushed
the
ci_portability_on_label
branch
from
48cd45a
to
06add6d
Compare
|
Documentation preview for this PR (built with commit 3c410d8; changes) is ready! 🎉 |
|
These would be run in addition to the usual CI jobs, right? And what they would test (beyond the usual CI) is the uninstall-and-reinstall process on top of an existing docker image? I'm sceptical that an incremental build will catch many problems that a build-from-scratch would miss, but I know it's happened before when dependencies or Makefile rules get messed up. And I don't mind wasting Github's money. So it LGTM if the Github voodoo works. |
Yes, this complements both the Build & Test (which runs on every PR and only does one platform, and does not do anything with upgrades of packages that it has taken from the system) and the CI Linux (which only runs on release tags or manual tagging for trigger the CI).
I'm using the incremental build to cut down the build time. The uninstall-and-reinstall game is to make sure that in the presence of usable system packages, the change actually gets tested.
Right, that's a minor additional useful thing that this checks. |
|
Thanks for reviewing. May I set it to "positive review"? |
Oh, that's the part I overlooked completely. OK. Yeah, I'll set it to positive review. I think there are cases where we should let old distros break if it makes sage developers' lives easier, but that eternal argument is happening elsewhere. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Sure, that's #32074. We just dropped some distros with old GCCs |
…: packages: standard" PRs
<!-- 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 runs a small subset of the Linux platforms that are part of our
portability suite for all PRs that are labeled "c: packages: standard"
or "c: packages: optional".
The build is run incrementally on top of the Docker images at ghcr.io
- Packages whose `build/pkgs/SPKG/configure.m4` is changed are first
uninstalled by running `make SPKG-uninstall`.
- For packages whose metadata in `build/pkgs/` or `pkgs/` is changed,
`make SPKG-ensure` is run; this builds the package when no suitable
system package is available. (In particular, it triggers the build of an
optional package SPKG.)
As an illustration, we update SPKGs `libbraiding` (standard), `kissat`
(optional)
Also we extend the triggers for ci-conda so that they run on PR syncs
when one of the trigger labels is present.
<!-- 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#31501
<!-- 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.
- [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! -->
A test run:
https://github.com/mkoeppe/sage/actions/runs/4922782397/jobs/8793892293
URL: sagemath#35380
Reported by: Matthias Köppe
Reviewer(s): github-actions[bot], Matthias Köppe, Michael Orlitzky, Tobias Diez
…: packages: standard" PRs
<!-- 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 runs a small subset of the Linux platforms that are part of our
portability suite for all PRs that are labeled "c: packages: standard"
or "c: packages: optional".
The build is run incrementally on top of the Docker images at ghcr.io
- Packages whose `build/pkgs/SPKG/configure.m4` is changed are first
uninstalled by running `make SPKG-uninstall`.
- For packages whose metadata in `build/pkgs/` or `pkgs/` is changed,
`make SPKG-ensure` is run; this builds the package when no suitable
system package is available. (In particular, it triggers the build of an
optional package SPKG.)
As an illustration, we update SPKGs `libbraiding` (standard), `kissat`
(optional)
Also we extend the triggers for ci-conda so that they run on PR syncs
when one of the trigger labels is present.
<!-- 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#31501
<!-- 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.
- [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! -->
A test run:
https://github.com/mkoeppe/sage/actions/runs/4922782397/jobs/8793892293
URL: sagemath#35380
Reported by: Matthias Köppe
Reviewer(s): github-actions[bot], Matthias Köppe, Michael Orlitzky, Tobias Diez
…: packages: standard" PRs
<!-- 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 runs a small subset of the Linux platforms that are part of our
portability suite for all PRs that are labeled "c: packages: standard"
or "c: packages: optional".
The build is run incrementally on top of the Docker images at ghcr.io
- Packages whose `build/pkgs/SPKG/configure.m4` is changed are first
uninstalled by running `make SPKG-uninstall`.
- For packages whose metadata in `build/pkgs/` or `pkgs/` is changed,
`make SPKG-ensure` is run; this builds the package when no suitable
system package is available. (In particular, it triggers the build of an
optional package SPKG.)
As an illustration, we update SPKGs `libbraiding` (standard), `kissat`
(optional)
Also we extend the triggers for ci-conda so that they run on PR syncs
when one of the trigger labels is present.
<!-- 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#31501
<!-- 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.
- [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! -->
A test run:
https://github.com/mkoeppe/sage/actions/runs/4922782397/jobs/8793892293
URL: sagemath#35380
Reported by: Matthias Köppe
Reviewer(s): github-actions[bot], Matthias Köppe, Michael Orlitzky, Tobias Diez
|
This failed on https://github.com/sagemath/sage/actions/runs/6139901053, output is mysterious but if I had to guess it has something to do with it being a package removal and not a change. |
|
Does this also pick up Python packages installed via |
… all `upstream_url`s.
<!-- ^^^^^
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 -->
https://pallini.di.uniroma1.it/changes24-28.txt
Also, motivated by a mistake I made while preparing this PR
(sagemath#36774 (comment)),
and previously in sagemath#35380 (see sagemath#36781), and countless other times, I am
changing all `upstream_url`s to use `VERSION` instead of the hardcoded
version number. This makes it less error-prone to use `sage -package
update SPKG NEW_VERSION` and `sage -package update-latest`. These
commands now also display the URL from which they are downloading the
tarball, and they warn if the `upstream_url` field does not contain the
`VERSION` variable.
(Exceptions: `ipykernel`, which is fixed in sagemath#36129, and `libbraiding`,
which is fixed in sagemath#36781)
Because some of the `upstream_url` need the version in a different
format, a mechanism to refer to the components of a version is added:
The strings `VERSION_MAJOR`, `VERSION_MINOR`, `VERSION_MICRO` can be
used. For added readability, instead of the naked strings, also the
notation `${VERSION_MAJOR}` etc. can be used. For example, `nauty`,
which previously had to use a hardcoded version number, now uses the
upstream URL pattern
`upstream_url=https://pallini.di.uniroma1.it/nauty${VERSION_MAJOR}_${VER
SION_MINOR}_${VERSION_MICRO}.tar.gz`
The syntax `${VERSION_MAJOR}`, borrowed from Bourne shell, matches what
sage-bootstrap already uses in
https://github.com/sagemath/sage/blob/develop/.upstream.d/10-SAGE_SERVER
<!-- 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: ...
-->
- Depends on sagemath#36771 (merged here)
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36774
Reported by: Matthias Köppe
Reviewer(s): David Coudert
<!-- ^^^^^ 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 --> As previously observed in sagemath#36283, the intended upgrade of libbraiding to 1.2 as part of sagemath#35380 was faulty, and as a result we have a `libbraiding-1.2.tar.gz` on our mirrors that is actually 1.1. Here we rectify the situation. <!-- 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#36781 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik
… all `upstream_url`s.
<!-- ^^^^^
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 -->
https://pallini.di.uniroma1.it/changes24-28.txt
Also, motivated by a mistake I made while preparing this PR
(sagemath#36774 (comment)),
and previously in sagemath#35380 (see sagemath#36781), and countless other times, I am
changing all `upstream_url`s to use `VERSION` instead of the hardcoded
version number. This makes it less error-prone to use `sage -package
update SPKG NEW_VERSION` and `sage -package update-latest`. These
commands now also display the URL from which they are downloading the
tarball, and they warn if the `upstream_url` field does not contain the
`VERSION` variable.
(Exceptions: `ipykernel`, which is fixed in sagemath#36129, and `libbraiding`,
which is fixed in sagemath#36781)
Because some of the `upstream_url` need the version in a different
format, a mechanism to refer to the components of a version is added:
The strings `VERSION_MAJOR`, `VERSION_MINOR`, `VERSION_MICRO` can be
used. For added readability, instead of the naked strings, also the
notation `${VERSION_MAJOR}` etc. can be used. For example, `nauty`,
which previously had to use a hardcoded version number, now uses the
upstream URL pattern
`upstream_url=https://pallini.di.uniroma1.it/nauty${VERSION_MAJOR}_${VER
SION_MINOR}_${VERSION_MICRO}.tar.gz`
The syntax `${VERSION_MAJOR}`, borrowed from Bourne shell, matches what
sage-bootstrap already uses in
https://github.com/sagemath/sage/blob/develop/.upstream.d/10-SAGE_SERVER
<!-- 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: ...
-->
- Depends on sagemath#36771 (merged here)
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36774
Reported by: Matthias Köppe
Reviewer(s): David Coudert
<!-- ^^^^^ 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 --> As previously observed in sagemath#36283, the intended upgrade of libbraiding to 1.2 as part of sagemath#35380 was faulty, and as a result we have a `libbraiding-1.2.tar.gz` on our mirrors that is actually 1.1. Here we rectify the situation. <!-- 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#36781 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik
mkoeppe
added
the
disputed
📚 Description
This runs a small subset of the Linux platforms that are part of our portability suite for all PRs that are labeled "c: packages: standard" or "c: packages: optional".
The build is run incrementally on top of the Docker images at ghcr.io
build/pkgs/SPKG/configure.m4is changed are first uninstalled by runningmake SPKG-uninstall.build/pkgs/orpkgs/is changed,make SPKG-ensureis run; this builds the package when no suitable system package is available. (In particular, it triggers the build of an optional package SPKG.)As an illustration, we update SPKGs
libbraiding(standard),kissat(optional)Also we extend the triggers for ci-conda so that they run on PR syncs when one of the trigger labels is present.
📝 Checklist
⌛ Dependencies
A test run: https://github.com/mkoeppe/sage/actions/runs/4922782397/jobs/8793892293