1. The directories .ci, .devcontainer, .github/workflows. These are special directories that control the GitHub workflows that run for example on pull requests and when release tags are pushed.2. The files tox.ini and build/bin/write-dockerfile.sh. They contain the infrastructure for portability testing of the Sage distribution (https://doc.sagemath.org/html/en/developer/portability_testing.html).
How about moving them out of the main Sage tree into separate repos, which can be accessed from the main tree as git submodules?
Oops. Experimenting with this idea, I accidentally pushed a "crazy" commit to "develop". Please revoke the commit ASAP!
have a CI/sage-distro repo [...] with all that .github/ etc stuff needed for CI, including a part of build/ - and checkout sagelib as a submodule.
[...] git submodules [...]
On 10 April 2024 19:24:12 CEST, Matthias Koeppe <matthia...@gmail.com> wrote:
>On Tuesday, April 9, 2024 at 3:28:27 PM UTC-7 Dima Pasechnik wrote:
>
>[...] git submodules [...]
>
>git submodules are included in a repository by specific commit sha of the
>submodule repo.
>So whenever one has to make a change in the submodule repo, one also has to
>commit a change (by a second PR) in the main repo.
Why a 2nd PR?
On Wed, Apr 10, 2024 at 9:02 PM Matthias Koeppe <matthia...@gmail.com> wrote:On Wednesday, April 10, 2024 at 1:00:06 PM UTC-7 Dima Pasechnik wrote:On 10 April 2024 19:24:12 CEST, Matthias Koeppe <matthia...@gmail.com> wrote:
>On Tuesday, April 9, 2024 at 3:28:27 PM UTC-7 Dima Pasechnik wrote:
>[...] git submodules [...]
>
>git submodules are included in a repository by specific commit sha of the
>submodule repo.
>So whenever one has to make a change in the submodule repo, one also has to
>commit a change (by a second PR) in the main repo.
Why a 2nd PR?I just explained it. To update the version (commit sha) of the submodule.Have you ever worked with submodules?
There is simply no need to keep the .gitsubmodules -the only file in the main repo affected by "git submodule update --remote" -always synchronized, commit-wise, for everyone.
On Wed, Apr 10, 2024 at 04:23:13PM -0700, Matthias Koeppe wrote:
> On Wednesday, April 10, 2024 at 3:25:16 PM UTC-7 Dima Pasechnik wrote:
> Necessary reminder that we're discussing, as the subject says, the files
> that control the GitHub workflows and the Codespaces.
> What a developer may do on their local machine does not matter.
But there is no difference here between a CI and a local machine here.
A CI is perfectly capable of doing
"git submodule update --remote"
and proceed.
I propose a governance change for a small part of the main Sage repository:1. The directories .ci, .devcontainer, .github/workflows. These are special directories that control the GitHub workflows that run for example on pull requests and when release tags are pushed, as well as the Dev Containers feature on GitHub.2. The file tox.ini. It contains the infrastructure for portability testing of the Sage distribution (https://doc.sagemath.org/html/en/developer/portability_testing.html).
3. The file src/doc/en/developer/portability_platform_table.rst (which I update using "tox -e update_docker_platforms").
Proposed change: All changes to these files are made through PRs. When the PR is ready, a developer in the Maintainer role directly merges the PR into the "develop" branch. In other words, switch to the "Show" model for these changes.
It was a bit unclear to me how your v2 proposal is different from the initial proposal on this sage-devel thread. Maybe it's helpful to clarify that build/bin/write-dockerfile.sh was removed from the proposal and src/doc/en/developer/portability_platform_table.rst was added. Otherwise, it's in essentially unchanged.
I propose a governance change for a small part of the main Sage repository:
1. The directories .ci, .devcontainer, .github/workflows. [...]2. The file tox.ini. [...]
3. The file src/doc/en/developer/portability_platform_table.rst (which I update using "tox -e update_docker_platforms").
I think we should restrict the scope to the files and directories strictly in the CI infrastructure. Anything under `src/` should be excluded.
Proposed change: All changes to these files are made through PRs. When the PR is ready, a developer in the Maintainer role directly merges the PR into the "develop" branch. In other words, switch to the "Show" model for these changes.I fear that developers in the Maintainer role could conflict in making changes to the CI-related files, as we suffered in the disputed PRs.
So I suggest to have only one Maintainer for those files. As we acknowledge a certain dictatorship of the release manager, we may have a vote to elect the CI manager and give him/her a restricted dictatorship and responsibility.
On Monday, May 6, 2024 at 10:49:07 PM UTC-7 Kwankyu Lee wrote:I propose a governance change for a small part of the main Sage repository:1. The directories .ci, .devcontainer, .github/workflows. [...]2. The file tox.ini. [...]3. The file src/doc/en/developer/portability_platform_table.rst (which I update using "tox -e update_docker_platforms").I think we should restrict the scope to the files and directories strictly in the CI infrastructure. Anything under `src/` should be excluded.This file src/doc/en/developer/portability_platform_table.rst is part of the documentation of the CI infrastructure and needs to be updated (by script) whenever I make changes to the tested platforms.
Dear Sage developers:I propose to consider the following governance change for a small part of the Sage repository:
1. The directories .ci, .devcontainer, .github/workflows. These are special directories that control the GitHub workflows that run for example on pull requests and when release tags are pushed.
2. The files tox.ini and build/bin/write-dockerfile.sh. They contain the infrastructure for portability testing of the Sage distribution (https://doc.sagemath.org/html/en/developer/portability_testing.html).
Some of these files are shipped as part of the Sage distribution, but none of them have any role in the build process or runtime of Sage, and thus none of them are tested by the Release Manager.Status quo: All changes to these files go through the normal review process for Sage PRs; when set to "positive review", Volker merges them into the next development release.In the terminology of https://martinfowler.com/articles/ship-show-ask.html (ht Gonzalo Tornaria), this is the "Ask" model.Acknowledgment: I'm grateful to all who have contributed to the review of my PRs that made changes to these files in the past: thanks for your time and energy.
Proposed change: All changes to these files are made through PRs. When the PR is ready, a developer in the Maintainer role directly merges the PR into the "develop" branch.In other words, switch to the "Show" model for these changes.
Why the change:1. Changes to these files do not have any effect on the build and runtime of Sage;- thus changes to these files do not risk breaking the mathematical correctness, or the performance of anything in Sage;- hence there may not be the same need for formal review compared to changes to the Sage library.2. Our project has a collective interest in smoothly operating development infrastructure / quality assurance tools;- but tragedy of the commons;- more specifically, developing/improving such development tools only pays off individually for developers with a sufficiently high volume of activity (cf. https://github.com/sagemath/sage/graphs/contributors?from=2020-01-01&to=2024-04-09&type=c);- there may also be a technical barrier that prevents developers from even reviewing a PR that makes changes to these files;- hence, waiting for reviewers to approve a PR and waiting for the Release Manager to merge it adds too much delay and friction.
3. Examples (all PRs authored by me, waiting for review):- "CI build, doc-build: Run containers explicitly, separate jobs for pyright, build, modularized tests, long tests" (https://github.com/sagemath/sage/pull/36498) waiting for review since Oct 21, 2023- "GH Actions: Build platform-independent wheels of sagemath-environment, sage-setup, sage-sws2rst for PyPI" (https://github.com/sagemath/sage/pull/37099) waiting for review since Feb 5- "CI: Update Linux platforms / runners" waiting for review since Feb 14- "GH Actions: Build macOS arm64 wheels" (https://github.com/sagemath/sage/pull/37503) waiting for review since Feb 28- "CI Build&Test: Fix "test modularized distributions" (https://github.com/sagemath/sage/pull/37750) waiting for review since Apr 4- "dist.yml: Download optional/experimental tarballs for GitHub Release assets" (https://github.com/sagemath/sage/pull/37762) waiting for review since Apr 64. Non-examples (all PRs authored by me, waiting for review):- "CI Build&Test: Show segfaults using GitHub annotations" (https://github.com/sagemath/sage/pull/37738, waiting for review) -- this makes changes in sage.doctest, so would continue to be reviewed normally- "tox.ini: Add environments ruff, ruff-minimal; GH Actions: run ruff-minimal" (https://github.com/sagemath/sage/pull/37453, waiting for review) -- this also makes changes in src/tox.ini and src/doc, so would continue to be reviewed normally- "src/tox.ini (coverage:run): Set concurrency = multiprocessing,threads" (https://github.com/sagemath/sage/pull/37010) -- makes changes in src/tox.ini, so would continue to be reviewed normally
The command "tox -e update_docker_platforms" involves a change of the list of tested platforms. The change become effective when a beta release is made by the release manager, since the docker image files are created only at the release time. So there is no need to merge early the change to the develop branch.On the other hand, is it an unnecessary friction that changes of the list of tested platforms go through the normal review process?
I've already said while the previous version of this was discussed, is
that it's a huge mess to have different commit rights for different
parts of the tree,
and I proposed to spin the CI into a separate
repository, as an alternative which simplifies a lot of things.
There are some means to reuse workflows in GitHub
(I have not checked if they are feasible for us) and one could certainly try to extract some things into shared actions that live elsewhere.
+1 to the general idea of maintainers for distinct subtrees
As far as the implementation, I'd rather follow a model where there is a single merge queue at the end (currently me, could be automated when the CI is stricter and developers do not / cannot ignore it any more). [...]
Really all you need is to
* have a "CI Build & Test" tag whose PR's are merged in before CI runs.
* maintainer sets positive review on tickets under his responsibility
If I read your proposal correctly, it is about removing review from changes made by "maintainers" [...]
Mostly, I am opposed to this because changes to the files you list are not automatically uncontroversial, see the disputed https://github.com/sagemath/sage/pull/37740, https://github.com/sagemath/sage/pull/37387, https://github.com/sagemath/sage/pull/37351, https://github.com/sagemath/sage/pull/36726, https://github.com/sagemath/sage/pull/36697, https://github.com/sagemath/sage/pull/36694, https://github.com/sagemath/sage/pull/36678, (there's probably more.)
[...] In general, I am not sure about changing the review requirements. I know that having to do several rounds of review can be frustrating. At the same time, I like the idea of having somebody double check things. (I consider waiting for the first round of review a necessary annoyance. What can be really frustrating is waiting for the second round of review. Maybe, there are other ways to improve this, e.g., by encouraging people to set things to "feel free to set to positive review yourself once you addressed these tiny things I brought up in my review.")