CI Linux: Fix "optional", "experimental" jobs #36668
Conversation
…a even when docker_push_repository is not set
|
Documentation preview for this PR (built with commit 9c8f3b6; changes) is ready! 🎉 |
| @@ -191,6 +191,9 @@ jobs: | |||
| echo "DOCKER_PUSH_REPOSITORY=$(echo ${{ inputs.docker_push_repository }} | tr "[:upper:]" "[:lower:]")" >> $GITHUB_ENV | |||
| echo "DOCKER_CONFIG_FILE=$HOME/.docker/config.json" >> $GITHUB_ENV | |||
| fi | |||
|
|
|||
| - name: Determine Docker tags to use | |||
| run: | | |||
| # This line needs to be run before the step "Merge CI fixes from sagemath/sage". | |||
There was a problem hiding this comment.
This fix seems to indicate that the previous step (logging to ghcr.io) is not really necessary (used) for subsequent steps. So perhaps the step is here to provide the environment variables to the calling workflow.
There was a problem hiding this comment.
The environment variables are set for the following steps in this job, specifically for invoking tox.
There was a problem hiding this comment.
I meant
(A): logging into docker and setting the environment variables DOCKER_PUSH_REPOSITORY and DOCKER_CONFIG_FILE
in the previous step to the "Determine Docker tags to use" step.
If the idea is to run "Determine Docker tags to use" step even when inputs.docker_push_repository is empty, then it means that (A) is not necessary for the steps following and including the "Determine Docker tags to use" step.
So I thought (A) could be moved out to the calling workflow and don't have to be in the docker workflow. I am not suggesting we should do it. I just want to know if I understand the situation correctly. Am I right?
There was a problem hiding this comment.
If the idea is to run "Determine Docker tags to use" step even when
inputs.docker_push_repositoryis empty, then it means that (A) is not necessary for the steps following and including the "Determine Docker tags to use" step.
That's right, the two parts now separated do not interact with each other. Each part sets some environment variables for the following steps.
So I thought (A) could be moved out to the calling workflow and don't have to be in the docker workflow. I am not suggesting we should do it. I just want to know if I understand the situation correctly. Am I right?
No; environment variables set by appending to the file $GITHUB_ENV only take effect for the next steps of a job. But a reusable workflow is run as a job, not a step of a job. See https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations
|
Thanks! |
|
Thanks for fixing this! In principle it seems to work now (for example on Debian Buster). But a lot of jobs have been cancelled due to timeout, especially the ones running on Ubuntu (example Ubuntu Xenial) In the log-files of successful jobs I noticed errors on testing the database_knotinfo installation (and other test suites of optional packages). For example in Ubuntu boinic: It looks like missing dependencies, no? |
There I see (after setting "Show timestamps"): This is a bug -- these two should not take 3 hours to complete. |
Thanks for catching this! Fixed in #36708 |
<!-- ^^^^^ 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 --> Fixes test failure of `database_knotinfo` seen in GH Actions, as reported by @soehms in sagemath#36668 (comment) Fixes same also for `database_cubic_hecke` and `mathics`. To test: `FROM_DOCKER_TARGET=with-targets-pre TARGETS_PRE=config.status tox -e docker-ubuntu-bionic-gcc_8-maximal-incremental -- database_knotinfo database_cubic_hecke mathics` We also remove packages starting with `_` from the list of tested packages. The dependencies of `_develop` pulled in the dummy package `github_cli`. We also use output groups to fold away some repetitive output. <!-- 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#36708 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik, Sebastian Oehms
Fixes #36667
📝 Checklist
⌛ Dependencies