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

New GitHub workflow to continue our images on Docker Hub #36047

Merged
merged 2 commits into from
Aug 27, 2023

Conversation

soehms
Copy link
Member

@soehms soehms commented Aug 8, 2023

This PR implements a GitHub workflow which sould establish a continuation of the Docker images in the Sage Docker Hub repositories. Therefore, it aims to replace $SAGE_ROOT/.gitlab-ci.yml which has been used formerly for this purpose but isn't working any more (see sage-devel-thread fom April 5th).

The new worklfow uses the same $SAGE_ROOT/docker/Dockerfile as SAGE_ROOT/.gitlab-ci.yml and is based on docker/build-push-action@v4.

Concerning the images placed in the sagemath-dev repository you should note the following difference to the releases built before 9.8: They are no longer build on target sagemath-dev since this causes a "No space left on device" error (see the log-file of a correspondig test run). Instead they are build on target make-build, now. I think the main difference is the missing build of documentation.

But, I don't know if this fits with the idea of this repository. If we really need target sagemath-dev then maybe something like the free disk space step of job linux in .github/workflows/docker.yml would help to get it run through. What about to just pull ghcr.io/sagemath/sage/sage-ubuntu-jammy-standard-with-targets and push it to sagemath-dev on Docker Hub. Please post here what you think would be the preferred way. Until this is not clearified I ommit setting the tags develop and latest in sagemath-dev (note that these tags still have ongoing pulls).

The workflow does not use docker/metadata-action@v4 as we need to distinguish between latest and develop tags in the Docker Hub repositories. Furthermore, if the workflow is triggered manually, this should result in the same tags as when triggered by pushing a new release tag (metadata would take the branch name).

The reason why the worklflow is split into two jobs is because the time limit of 6 hours per job was exeeded in a former test with just one job.

I've tested the new workflow in my fork repository using the workflow_dispatch (see the corresponding log-file). The resulting tags on Docker Hub are:

In order that this worked I had to set the secrets secrets.DOCKERHUB_USERNAME and secrets.DOCKERHUB_TOKEN in my fork of the GitHub repository. Thus, after this PR is merged a maintainer has to set this secrets on the upstream repository.

📝 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


################################################################################
# Image containing the run-time dependencies for Sage #
################################################################################
FROM ubuntu:jammy as run-time-dependencies
FROM ubuntu:latest as run-time-dependencies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not use latest here. In the past things usually broke when ubuntu upgraded their "latest" tag.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will change that in the next commit!

Comment on lines +67 to +69
- name: Set up QEMU
uses: docker/setup-qemu-action@v2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need QEMU here and below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need QEMU here and below?

It's there from the master I used. I will drop it in the next commit.

@saraedum
Copy link
Member

saraedum commented Aug 9, 2023

I am happy with these changes if they make the docker build work again. (I had some minor comments, but nothing of much substance.)

Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
@soehms
Copy link
Member Author

soehms commented Aug 9, 2023

I am happy with these changes if they make the docker build work again. (I had some minor comments, but nothing of much substance.)

Thanks for the review!

Can you please elaborate on my (implicit) question from the paragraph starting with

But, I don't know if this fits with the idea of this repository...

More precisely, will it be o.K. to change the meaning of the sagemath-dev repository since I'm now pushing target make-build there instead of target sagemath-dev formerly? Moreover, will it be o.K. to push make-build as tags develop resp. latest there, as well? I see that there are still pulls on these tags (maybe from some out of date-CI-job). But perhaps that will change some expectations.

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Documentation preview for this PR (built with commit b0d14d2; changes) is ready! 🎉

@mkoeppe
Copy link
Member

mkoeppe commented Aug 13, 2023

Let's merge it so it can run automatically on the release.

@soehms
Copy link
Member Author

soehms commented Aug 15, 2023

Let's merge it so it can run automatically on the release.

Thank you very much! I agree: the sooner it's up and running, the better. I will open a follow-up PR in September to cover @saraedum 's suggestions. Do we already have Docker Hub secrets in the upstream repository?

@saraedum
Copy link
Member

But, I don't know if this fits with the idea of this repository...

I have to admit I don't really understand your questions. The meaning of the repositories and tags is explained in the README at https://hub.docker.com/r/sagemath/sagemath-dev/.

Does that answer the question about develop and latest?

So, what's in sagemath-dev exactly? It's a bit hard to say. It was meant as a development environment but I don't think it was ever much used for that purpose. So some CI jobs use it because you can patch SageMath and quickly rebuild and (more importantly) sage -i is fully functional unlike in the sagemath repository.

Does sage -i still work in your sagemath-dev?

@soehms
Copy link
Member Author

soehms commented Aug 17, 2023

But, I don't know if this fits with the idea of this repository...

I have to admit I don't really understand your questions. The meaning of the repositories and tags is explained in the README at https://hub.docker.com/r/sagemath/sagemath-dev/.

Yes, I've seen that.

Does that answer the question about develop and latest?

No! My point about the tags relates to your answer below. Since they're still in use, I've simply avoided setting them before realizing how much current behavior will change.

So, what's in sagemath-dev exactly? It's a bit hard to say. It was meant as a development environment but I don't think it was ever much used for that purpose. So some CI jobs use it because you can patch SageMath and quickly rebuild and (more importantly) sage -i is fully functional unlike in the sagemath repository.

Does sage -i still work in your sagemath-dev?

In principle it works, but unfortunately a large part of Sage had to be rebuilt. Is this what the earlier sagemath-dev images should have avoided? If you're currently running a container on sagemath-dev:9.7, a similar rebuild of Sage will be performed even before you reach the bash prompt. So it seems that there will be no real regression on this point.

My suggestion: After my vacation (around mid-September) I manually replace the tag develop with the current image that the new job pushed. If there are no objections after a few weeks, I will adjust the GitHub workflow accordingly.

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 23, 2023
…r Hub

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

This PR implements a GitHub workflow which sould establish a
continuation of the Docker images in the Sage Docker Hub repositories.
Therefore, it aims to replace `$SAGE_ROOT/.gitlab-ci.yml` which has been
used formerly for this purpose but isn't working any more (see [sage-
devel-thread fom April 5th](https://groups.google.com/g/sage-
devel/c/eJ_OicQXM8Y)).

The new worklfow uses the same `$SAGE_ROOT/docker/Dockerfile` as
`SAGE_ROOT/.gitlab-ci.yml` and is based on [docker/build-push-
action@v4](https://github.com/docker/build-push-action).

Concerning the images placed in the [sagemath-dev
repository](https://hub.docker.com/r/sagemath/sagemath-dev) you should
note the following difference to the releases built before 9.8: They are
no longer build on target `sagemath-dev` since this causes a "No space
left on device" error (see the [log-
file](https://github.com/soehms/sage/actions/runs/5694284990) of a
correspondig test run). Instead they are build on target `make-build`,
now. I think the main difference is the missing build of documentation.

But, I don't know if this fits with the idea of this repository. If we
really need target `sagemath-dev` then maybe something like the `free
disk space` step of job `linux` in `.github/workflows/docker.yml` would
help to get it run through. What about to just pull
`ghcr.io/sagemath/sage/sage-ubuntu-jammy-standard-with-targets` and push
it to `sagemath-dev` on Docker Hub. Please post here what you think
would be the preferred way. Until this is not clearified I ommit setting
the tags `develop` and `latest` in `sagemath-dev` (note that these tags
still have ongoing pulls).

The workflow does not use [docker/metadata-
action@v4](https://github.com/docker/metadata-action#latest-tag) as we
need to distinguish between `latest` and `develop` tags in the Docker
Hub repositories. Furthermore, if the workflow is triggered manually,
this should result in the same tags as when triggered by pushing a new
release tag (`metadata` would take the branch name).

The reason why the worklflow is split into two jobs is because the time
limit of 6 hours per job was exeeded in a former test with just one job.

I've tested the new workflow in my fork repository using the
`workflow_dispatch` (see the corresponding [log-
file](https://github.com/soehms/sage/actions/runs/5795823327)). The
resulting tags on Docker Hub are:


* [sagemath/sagemath:10.1.beta9](https://hub.docker.com/layers/sagemath/
sagemath/10.1.beta9/images/sha256-
4c00148d6e6c2134728b0a74d0e778281d64a8f0ff9b49ade0b9af68de758f12?context
=repo)
* [sagemath/sagemath:develop](https://hub.docker.com/layers/sagemath/sag
emath/develop/images/sha256-
4c00148d6e6c2134728b0a74d0e778281d64a8f0ff9b49ade0b9af68de758f12?context
=repo)
* [sagemath/sagemath-
dev:10.1.beta9](https://hub.docker.com/layers/sagemath/sagemath-dev/10.1
.beta9/images/sha256-
d5071d0cb683edf9f22d19c2c26d5d580dc4e05823c755bfce85c744b557fd14?context
=repo)

In order that this worked I had to set the secrets
`secrets.DOCKERHUB_USERNAME` and `secrets.DOCKERHUB_TOKEN` in my fork of
the GitHub repository. Thus, after this PR is merged a maintainer has to
set this secrets on the upstream repository.


<!-- 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#36047
Reported by: Sebastian Oehms
Reviewer(s): Julian Rüth, Sebastian Oehms
@vbraun vbraun merged commit 8f938b7 into sagemath:develop Aug 27, 2023
13 of 15 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Aug 27, 2023
@soehms
Copy link
Member Author

soehms commented Sep 12, 2023

My suggestion: After my vacation (around mid-September) I manually replace the tag develop with the current image that the new job pushed. If there are no objections after a few weeks, I will adjust the GitHub workflow accordingly.

This is done, now. I've triggered the docker images for 10.1, 10.2.beta1 and 10.2.beta2 manually again via my fork repository.

It seems that we still don't have the DockerHub secrets in the GitHub sagemath/sage repository. The workflow has been triggered for all beta releases since10.2.beta0 but all runs failed at login (for example in the 10.2.beta2 log-file):

Run docker/login-action@v2
  with:
    ecr: auto
    logout: true
  env:
    TAG_NAME: 10.2.beta2
    TAG: sagemath/sagemath-dev:10.2.beta2
    TAG_LIST: sagemath/sagemath-dev:10.2.beta2
Error: Username and password required

Since I can't set the secrets, can please one of you, @saraedum @mkoeppe, do that?

@saraedum
Copy link
Member

I added some secrets. Can you try if that works?

@soehms
Copy link
Member Author

soehms commented Sep 14, 2023

I added some secrets. Can you try if that works?

Thanks a lot! Unfortunately I can't even trigger the workflow manually in the sagemath/sage repository (by missing permissions). Maybe you can do it here. In my fork repository it looks like this:

grafik

If the develop branch is not pre-selected you should change the selection accordingly and than push the button.

But, if you have trouble, we can just wait until the next beta release (push --tags event). Two secrets are needed: DOCKERHUB_USERNAME = sagemath and a Docker Hub token in DOCKERHUB_TOKEN. If these are set, it should work.

@soehms
Copy link
Member Author

soehms commented Sep 17, 2023

I added some secrets. Can you try if that works?

Thanks again! The login is working now (see the logfile of the last beta release). The reason that the workflow did fail again was a problem with building scipy-1.11.2 (see this post on sage-release).

@soehms
Copy link
Member Author

soehms commented Oct 2, 2023

I added some secrets. Can you try if that works?

Thanks again! The login is working now (see the logfile of the last beta release). The reason that the workflow did fail again was a problem with building scipy-1.11.2 (see this post on sage-release).

Now, with 10.2.beta5 we had the first successful auto-generated images on Docker Hub by this workflow (see for example sagemath/sagemath:develop)

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 15, 2023
    
<!-- ^^^^^
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. -->

This is a follow up PR of sagemath#36047. It does the following:

1. It fixes a wrong sort order for the tags that would cause a wrong tag
for stable releases
2. It enables the push of the `sagemath-dev:develop` and `sagemath-
dev:latest` tags according to my suggestion in
sagemath#36047 (comment)
3. It implements improvements suggested by @saraedum in sagemath#36047.
4. It contains the upgrades from the auto-generated *dependabot-
branches* for the Docker Hub GitHub App
(`dependabot/github_actions/docker/...`). Thus, as soon as this PR is
merged, these auto-generated branches can be deleted by a maintainer.

Still open is this suggestion from
sagemath#36047 (comment):

> Could we delete the gitlab setup in this PR as well? It's not working
anymore anyway

@saraedum What should be deleted explicitly? `.gitlab-ci.yml` and the
scripts used by it excludingly (`.ci/protect-secrets.sh`, `.ci/describe-
system.sh`)? Other sources?


### 📝 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#36385
Reported by: Sebastian Oehms
Reviewer(s): Matthias Köppe, Sebastian Oehms
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 17, 2023
    
<!-- ^^^^^
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. -->

This is a follow up PR of sagemath#36047. It does the following:

1. It fixes a wrong sort order for the tags that would cause a wrong tag
for stable releases
2. It enables the push of the `sagemath-dev:develop` and `sagemath-
dev:latest` tags according to my suggestion in
sagemath#36047 (comment)
3. It implements improvements suggested by @saraedum in sagemath#36047.
4. It contains the upgrades from the auto-generated *dependabot-
branches* for the Docker Hub GitHub App
(`dependabot/github_actions/docker/...`). Thus, as soon as this PR is
merged, these auto-generated branches can be deleted by a maintainer.

Still open is this suggestion from
sagemath#36047 (comment):

> Could we delete the gitlab setup in this PR as well? It's not working
anymore anyway

@saraedum What should be deleted explicitly? `.gitlab-ci.yml` and the
scripts used by it excludingly (`.ci/protect-secrets.sh`, `.ci/describe-
system.sh`)? Other sources?


### 📝 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#36385
Reported by: Sebastian Oehms
Reviewer(s): Matthias Köppe, Sebastian Oehms
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 19, 2023
    
<!-- ^^^^^
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. -->

This is a follow up PR of sagemath#36047. It does the following:

1. It fixes a wrong sort order for the tags that would cause a wrong tag
for stable releases
2. It enables the push of the `sagemath-dev:develop` and `sagemath-
dev:latest` tags according to my suggestion in
sagemath#36047 (comment)
3. It implements improvements suggested by @saraedum in sagemath#36047.
4. It contains the upgrades from the auto-generated *dependabot-
branches* for the Docker Hub GitHub App
(`dependabot/github_actions/docker/...`). Thus, as soon as this PR is
merged, these auto-generated branches can be deleted by a maintainer.

Still open is this suggestion from
sagemath#36047 (comment):

> Could we delete the gitlab setup in this PR as well? It's not working
anymore anyway

@saraedum What should be deleted explicitly? `.gitlab-ci.yml` and the
scripts used by it excludingly (`.ci/protect-secrets.sh`, `.ci/describe-
system.sh`)? Other sources?


### 📝 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#36385
Reported by: Sebastian Oehms
Reviewer(s): Matthias Köppe, Sebastian Oehms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants