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

GH Actions: Create daily integration branch #33222

Closed
mkoeppe opened this issue Jan 23, 2022 · 110 comments
Closed

GH Actions: Create daily integration branch #33222

mkoeppe opened this issue Jan 23, 2022 · 110 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jan 23, 2022

We create a GH Actions workflow, run daily (intended for running on sagetrac-mirror) as a cron job (
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule).

It can also be triggered manually via workflow_dispatch.

Example run at
https://github.com/mkoeppe/sage/runs/5060070979

  1. Merge mergeable Trac tickets from the current milestone, ordered by priority/deps, using git releasemgr merge-all --milestone=current

  2. Run using tox/docker on a stable platform (ubuntu-focal-recommended). (The new package factor recommended is like standard, but with texlive etc. added, as needed for doc-pdf). We set SAGE_DOCTEST_RANDOM_SEED=0 to reduce random failures.
    Only testsuite failures that do not already fail in the baseline are considered failures.

  3. Docker images are pushed to ghcr.io, see https://github.com/mkoeppe?tab=packages&q=recommended

Related issues and PRs:

Refinements:

  • Provide it with Trac credentials so it can set tickets back to needs_work on merge failures (using git releasemgr merge-all: Put tickets with merge conflict to needs_work WITH useful info git-trac-command#57)

    --> Trac account requested

  • Run git log --first-parent on files with merge conflict

  • Provide it with GitHub credentials so it can push a branch $RELEASE_TAG+integration-$DATE, if it passes build + doctests + doc-pdf.

  • Perhaps git bisect --first-parent for passing; set first failing ticket back to needs_work

  • Another workflow for integration testing of all tickets in the current milestone with --patchbot-status=Spkg and --status=needs_review positive_review with the full portability CI.

Depends on #33233
Depends on #31529
Depends on #33103
Depends on #33277

CC: @vbraun @dimpase

Component: distribution

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/gh_actions__create_daily_integration_branch @ 37c76a6

Issue created by migration from https://trac.sagemath.org/ticket/33222

@mkoeppe mkoeppe added this to the sage-9.6 milestone Jan 23, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 23, 2022

comment:1

Volker, would you be willing to share your existing scripts that probably already implement something like step 2 above? It does not have to be pretty.

@vbraun
Copy link
Member

vbraun commented Jan 23, 2022

comment:2

Scripts are part of https://github.com/sagemath/git-trac-command:

$ git releasemgr merge-all -h
usage: git-releasemgr merge-all [-h] [--limit LIMIT]

optional arguments:
  -h, --help     show this help message and exit
  --limit LIMIT  Merge this many tickets

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 23, 2022

comment:3

Thanks for the pointer

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 24, 2022

Dependencies: #30933

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 25, 2022

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 25, 2022

Commit: e803539

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 25, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e803539.github/workflows/integration.yml: New

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 25, 2022

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 26, 2022

Changed dependencies from #30933 to #30933, #33233

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 26, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7db353e.github/workflows/integration.yml: New
6d30ef9src/sage/tests/cmdline.py: Add diagnostics to a test that often fails
73450a8src/bin/sage-runtests: New option --baseline-stats-path
3551e19src/sage/doctest/control.py: Load base line stats
09ce9e0src/sage/doctest/reporting.py: No error status for failures already seen in baseline
3eecc40Merge #33233
edecb85.github/workflows/integration.yml: Check out from current repo, use sagetrac-mirror remote for develop and tags
c452317.github/workflows/integration.yml: Check out from current repo, use sagetrac-mirror remote for develop and tags

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 26, 2022

Changed commit from e803539 to c452317

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

1903e03.github/workflows/integration.yml: First build baseline, then test integration head
45af154.github/workflows/integration.yml: Make git available in the container
e4c3890.github/workflows/integration.yml: Add .git
7513533.github/workflows/integration.yml: Actually build baseline

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2022

Changed commit from c452317 to 7513533

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 27, 2022

Changed dependencies from #30933, #33233 to #30933, #33233, #31529

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2022

Changed commit from 7513533 to ec1f7c1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

96771be.github/workflows/integration.yml: Run 'git reset --hard' in the container so that non-ADDed files are not shown as deleted
9ddbcd1.github/workflows/integration.yml: Do not fail when baseline ptest has failures
5d35b97WIP
9dceb34fix M4 bugs
65d67d1build/pkgs/texlive/distros: Add *.txt
b09d0c7Merge #31529
ec1f7c1.github/workflows/integration.yml: Install texlive

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 5, 2022

comment:69

Replying to @tobiasdiez:

Replying to @mkoeppe:

I am deliberately setting this up as an image build, not just a container run, because I want to create a Docker image that can be pulled for local inspection.

Can you expand on why this should be helpful, i.e. where are the advantages over just checking out the branch in your local git (and maybe running it in the baseline-integration docker image if you like).

The build & test can take long. If it is prebuilt on GH Actions and I can download it for inspection, that's a benefit.

Also you can just execute the github workflow locally using act if you like.

I don't like

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 5, 2022

comment:70

Replying to @tobiasdiez:

Also, are you aware that there are special actions for the login and publish

Yes, but they don't help much. The login action could replace these 5 lines:

            TOKEN="${{ secrets.DOCKER_PKG_GITHUB_TOKEN }}"
            if [ -z "$TOKEN" ]; then
              TOKEN="${{ secrets.GITHUB_TOKEN }}"
            fi
            if echo "$TOKEN" | docker login ghcr.io -u ${{ github.actor }} --password-stdin; then

And the other one is not useful

@tobiasdiez
Copy link
Contributor

comment:71

Replying to @mkoeppe:

Replying to @tobiasdiez:

Replying to @mkoeppe:

This is to run make doc-pdf, an essential step that currently no CI is testing.

If it's so important, why not add it as a new target to the existing CI?

Yes, changing the existing CI to build the -recommended instead of -standard workflow may be something that we want to do at some point in the future. But this will need #33062 (split the workflow, with caching of Docker layers). That's exactly what the changes to tox.ini here on the ticket are implementing!

Can't change everything at the same time.

Okay, makes sense. Thanks for the explanation.

@tobiasdiez
Copy link
Contributor

comment:72

Replying to @mkoeppe:

Replying to @tobiasdiez:

Replying to @mkoeppe:

I am deliberately setting this up as an image build, not just a container run, because I want to create a Docker image that can be pulled for local inspection.

Can you expand on why this should be helpful, i.e. where are the advantages over just checking out the branch in your local git (and maybe running it in the baseline-integration docker image if you like).

The build & test can take long. If it is prebuilt on GH Actions and I can download it for inspection, that's a benefit.

But the push happens before the final "Build & test integration head", so it neither contains the test results nor the build, or I'm missing something?

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 5, 2022

comment:73

Yes, that's true as it is in the current branch - but that final step can be extended to also build; just not done yet

@tobiasdiez
Copy link
Contributor

comment:74

Replying to @mkoeppe:

Replying to @tobiasdiez:

Also, are you aware that there are special actions for the login and publish

Yes, but they don't help much. The login action could replace these 5 lines:

            TOKEN="${{ secrets.DOCKER_PKG_GITHUB_TOKEN }}"
            if [ -z "$TOKEN" ]; then
              TOKEN="${{ secrets.GITHUB_TOKEN }}"
            fi
            if echo "$TOKEN" | docker login ghcr.io -u ${{ github.actor }} --password-stdin; then

Yes, the docker/login-action is not doing much more: https://github.com/docker/login-action/blob/17f28ab24d0d2832d5ff23a1409bbfc373ebcb96/src/docker.ts#L30-L50 Except a bit of proper error handling and logging you out again. Anyway, why not reuse something official so that you don't have to worry about it?

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 5, 2022

comment:75

Because of the lines that follow if.

@tobiasdiez
Copy link
Contributor

comment:76

I guess it's getting late here for me, but your messages get more and more cryptic. There are a lot of ifs here...

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 5, 2022

comment:77

Sorry, I meant the last if but it also applies to the first if ;)

@dimpase
Copy link
Member

dimpase commented Feb 5, 2022

comment:78

Replying to @mkoeppe:

Also you can just execute the github workflow locally using act if you like.

wow, I never knew about act (guess it's https://github.com/nektos/act)

It should be in our developer manual.

@tobiasdiez
Copy link
Contributor

comment:79

Replying to @dimpase:

Replying to @mkoeppe:

Also you can just execute the github workflow locally using act if you like.

wow, I never knew about act (guess it's https://github.com/nektos/act)

Yes, that's the one (should have directly provided a link, sry). It's quite powerful and usually works really well. Don't know how Matthias can work on the github workflows without quickly checking locally if everything goes well.

@tobiasdiez
Copy link
Contributor

comment:80

Replying to @mkoeppe:

Sorry, I meant the last if but it also applies to the first if ;)

Even after sleep I don't understand your comment. The second if doesn't do anything in my opinion since the Github token is always there and we need to login to ghcr as you said earlier.
For the first if, this should be password = ${{ secrets.DOCKER_PKG_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} with the docker/login-action

@tobiasdiez
Copy link
Contributor

comment:81

Replying to @mkoeppe:

Replying to @tobiasdiez:

Replying to @mkoeppe:

This is to run make doc-pdf, an essential step that currently no CI is testing.

If it's so important, why not add it as a new target to the existing CI?

Yes, changing the existing CI to build the -recommended instead of -standard workflow may be something that we want to do at some point in the future.

Thinking about this again: would it be an idea to run the integration workflow as follows:

  • use the docker image build during the ci for the develop branch
  • merge all new tickets
  • re-build sage, including the packages that are needed for doc-pdf
  • run tests (using the ci artifact as baseline)
  • build docs

Not sure how long the last step takes, but step 4 should finish in 30-60 mins (depending on how much packages are touched in the to-be-merged tickets) and step 5 about 1h. So this should all fit very nicely in one workflow. So the main advantage would be that you don't have to rebuild all packages from scratch during the integration workflow.

This is under the assumption that the packages needed for the pdf build of the documentation don't influence the tests, because otherwise the test-baseline could be wrong (but is probably still good enough).

@vbraun
Copy link
Member

vbraun commented Feb 5, 2022

comment:82

Just FYI, I'm planning on adding a CI step that runs for each ticket at #33294. Sufficiently different from this ticket that both have merits, just a heads up.

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 5, 2022

comment:83

Replying to @tobiasdiez:

Don't know how Matthias can work on the github workflows without quickly checking locally if everything goes well.

With a lot of patience!

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 5, 2022

comment:84

Replying to @tobiasdiez:

  • re-build sage, including the packages that are needed for doc-pdf

these are system packages, not sage packages

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 5, 2022

comment:85

Replying to @mkoeppe:

changing the existing CI to build the -recommended instead of -standard workflow may be something that we want to do at some point in the future.

Preparation for that is now in #33296

@tobiasdiez
Copy link
Contributor

comment:86

Mainly to better understand the process behind the scenes, why is an additional "integration" branch necessary? Can one not simply push to "develop" (in case there are no errors)? I guess this comes down to the question "what does the release manager has to do in the background that demands a manual merge into the develop branch".

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 10, 2023

I'm guessing this is not needed

@mkoeppe mkoeppe closed this as not planned Won't fix, can't repro, duplicate, stale Feb 10, 2023
@mkoeppe mkoeppe removed this from the sage-9.9 milestone Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants