Opened 12 months ago

Closed 9 months ago

#31064 closed defect (fixed)

ci-cygwin*.yml: delegate to tox, add more stages, use more specific SAGE_LOCAL

Reported by: mkoeppe Owned by:
Priority: critical Milestone: sage-9.3
Component: porting: Cygwin Keywords:
Cc: slabbe, gh-kliem, arojas, embray, slelievre Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 42f4458 (Commits, GitHub, GitLab) Commit: 42f4458e3ae857f82759b3c9490ab34306a15f4d
Dependencies: #29124, #30944, #31062, #31084 Stopgaps:

Status badges

Description (last modified by mkoeppe)

We revise the CI workflows for Cygwin, now going through SAGE_ROOT/tox.ini for the package installation and invoking the new system package scripts from there.

We also add more stages to make the workflow more robust; this was originally done in #29152.

We also make another improvement that was easy to do: The build now uses SAGE_LOCAL=/opt/sage-$COMMIT instead of /sage/local. This will make it easier to download and install several versions of Sage.

Change History (53)

comment:1 Changed 12 months ago by mkoeppe

  • Dependencies changed from #29124 to #29124, #30944

comment:2 Changed 12 months ago by mkoeppe

  • Branch set to u/mkoeppe/ci_cygwin__yml__adjust_to_new_script_packages__bootstrap___prereq

comment:3 Changed 12 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc gh-kliem added
  • Commit set to b2ae68aea3cb706650731b0488b59ce992234e7b
  • Description modified (diff)
  • Reviewers set to https://github.com/mkoeppe/sage/actions/runs/427378215
  • Status changed from new to needs_review

Last 10 new commits:

898758dtox.ini: Add local-root
559dd8etox.ini (local-root): Pass --enable-build-as-root to configure
a85f41ctox.ini (local): Do not build the toolchain when posargs = config.status or posargs = configure
1de912atox.ini (local-sudo): Run apt-get update with sudo
3c7e5c4tox.ini (local-root, local-sudo): Run output of sage-print-system-package-command through eval
f7ff30cMerge branch 't/29124/script-packages-prereq-toolchain-bootstrap' into t/30944/tox__improve_local_sudo_ubuntu_standard
e9ca2c1Merge branch 't/29124/script-packages-prereq-toolchain-bootstrap' into t/30944/tox__improve_local_sudo_ubuntu_standard
be4177bMerge branch 't/30944/tox__improve_local_sudo_ubuntu_standard' into t/31064/ci_cygwin__yml__adjust_to_new_script_packages__bootstrap___prereq
f74fe3dtox.ini (local-cygwin-choco): New
b2ae68a.github/workflows/ci-cygwin*.yml: Install cygwin packages via tox

comment:4 Changed 12 months ago by mkoeppe

  • Status changed from needs_review to needs_work
  • Work issues set to SAGE_LOCAL

comment:5 Changed 12 months ago by git

  • Commit changed from b2ae68aea3cb706650731b0488b59ce992234e7b to 11cf8db0b49607b83c66d0d0d49ec52ea0069f39

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

11cf8db.github/workflows/ci-cygwin*.yml: Use SAGE_LOCAL as set by tox

comment:6 Changed 12 months ago by mkoeppe

  • Reviewers changed from https://github.com/mkoeppe/sage/actions/runs/427378215 to https://github.com/mkoeppe/sage/actions/runs/429356144
  • Status changed from needs_work to needs_review
  • Work issues SAGE_LOCAL deleted

comment:7 Changed 12 months ago by mkoeppe

  • Status changed from needs_review to needs_work
  • Work issues set to update extract-sage-local.sh

comment:8 Changed 11 months ago by git

  • Commit changed from 11cf8db0b49607b83c66d0d0d49ec52ea0069f39 to 05d57c0bf907fb91f229325f5cfa0e35b001fea2

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

05d57c0.github/workflows/ci-cygwin-*.yml: Pass PREFIX to tox

comment:9 Changed 11 months ago by git

  • Commit changed from 05d57c0bf907fb91f229325f5cfa0e35b001fea2 to d87e1604ba0c6a27ffc0e048d1bb351fadc54d90

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

d87e160.github/workflows/extract-sage-local.sh: Use PREFIX

comment:10 Changed 11 months ago by mkoeppe

  • Reviewers changed from https://github.com/mkoeppe/sage/actions/runs/429356144 to https://github.com/mkoeppe/sage/actions/runs/431265667

comment:11 Changed 11 months ago by git

  • Commit changed from d87e1604ba0c6a27ffc0e048d1bb351fadc54d90 to b327031d96686ead49e2386c180fedabd09d2a4e

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

0827c85Quoting fixes
00a9182Just use github.sha
b327031.github/workflows/extract-sage-local.sh: Fix up path

comment:12 Changed 11 months ago by mkoeppe

  • Reviewers changed from https://github.com/mkoeppe/sage/actions/runs/431265667 to https://github.com/mkoeppe/sage/runs/1580222034
  • Status changed from needs_work to needs_review
  • Work issues update extract-sage-local.sh deleted

comment:13 Changed 11 months ago by mkoeppe

  • Description modified (diff)

comment:14 Changed 11 months ago by mkoeppe

  • Dependencies changed from #29124, #30944 to #29124, #30944, #31062

comment:15 Changed 11 months ago by git

  • Commit changed from b327031d96686ead49e2386c180fedabd09d2a4e to a0d99d78c0d268552f2cf57ed86cef3548995521

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

9ad611dtox.ini, build/bin/write-dockerfile.sh: Add !sage_sws2rst, !rpy2 to SAGE_CHECK_PACKAGES
48657d2build/pkgs/sage_sws2rst/dependencies: Fix typo - use SAGE_CHECK_sage_sws2rst
380c718Merge branch 't/30944/tox__improve_local_sudo_ubuntu_standard' into t/31062/tox___gh_actions__disable_testsuites_of_packages_depending_on_pip_packages__pytest_______if_there_is_no_ssl
b25ea3bbuild/pkgs/rpy2/dependencies: If SAGE_CHECK=yes, depend on ipython
a0d99d7Merge branch 't/31062/tox___gh_actions__disable_testsuites_of_packages_depending_on_pip_packages__pytest_______if_there_is_no_ssl' into t/31064/ci_cygwin__yml__adjust_to_new_script_packages__bootstrap___prereq

comment:16 Changed 11 months ago by mkoeppe

Merged #31062 to fix a merge conflict

comment:17 Changed 11 months ago by git

  • Commit changed from a0d99d78c0d268552f2cf57ed86cef3548995521 to cf31b79036db96b5fd9e0b22f44c3692bc22356b

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

cea4cd5ci-cygwin-standard.yml: More stages, continue-on-error: true
cf31b79Fixup after cherry-pick

comment:18 Changed 11 months ago by mkoeppe

  • Reviewers changed from https://github.com/mkoeppe/sage/runs/1580222034 to https://github.com/mkoeppe/sage/actions/runs/432758069

Integrated the workflow changes from #29152

comment:19 Changed 11 months ago by mkoeppe

  • Cc arojas added

comment:20 Changed 11 months ago by mkoeppe

  • Dependencies changed from #29124, #30944, #31062 to #29124, #30944, #31062, #31084

comment:21 Changed 11 months ago by git

  • Commit changed from cf31b79036db96b5fd9e0b22f44c3692bc22356b to 8769bd66363a757f8917f8b59c149b4807649234

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

09d1737Makefile: Add targets ptest-nodoc etc.
0dca776Merge branch 't/29124/script-packages-prereq-toolchain-bootstrap' into t/31084/makefile__add__ptest__targets_that_do_not_depend_on_the_docbuild
073124cMerge branch 't/31084/makefile__add__ptest__targets_that_do_not_depend_on_the_docbuild' into t/31064/ci_cygwin__yml__adjust_to_new_script_packages__bootstrap___prereq
8769bd6.github/workflows/ci-cygwin-*.yml: Separate docbuild and ptest

comment:22 Changed 11 months ago by slelievre

  • Cc embray slelievre added

comment:23 Changed 11 months ago by git

  • Commit changed from 8769bd66363a757f8917f8b59c149b4807649234 to d2e4b830e489e11a9c6f1d530b417c996bd1662d

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

d2e4b83Fix up stage iv

comment:24 Changed 11 months ago by git

  • Commit changed from d2e4b830e489e11a9c6f1d530b417c996bd1662d to 38262228db0391e9bc8cda5bf06982d6d0db12ca

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

3826222Fix up stage iv

comment:25 Changed 11 months ago by mkoeppe

  • Summary changed from ci-cygwin*.yml: Adjust to new script packages _bootstrap, _prereq to ci-cygwin*.yml: Adjust to new script packages _bootstrap, _prereq, delegate to tox

comment:26 Changed 11 months ago by mkoeppe

  • Reviewers changed from https://github.com/mkoeppe/sage/actions/runs/432758069 to https://github.com/mkoeppe/sage/actions/runs/434342215

comment:27 Changed 11 months ago by mkoeppe

  • Work issues set to cygwin-minimal: don't pick up mingw tools

comment:28 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:29 Changed 11 months ago by mkoeppe

  • Status changed from needs_work to needs_review
  • Work issues cygwin-minimal: don't pick up mingw tools deleted

comment:30 Changed 11 months ago by git

  • Commit changed from 38262228db0391e9bc8cda5bf06982d6d0db12ca to 598b0c233a501dba64b66c26577d533c826d9ad5

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

598b0c2local-cygwin-choco: Do not pass through PATH, use full pathname of choco instead

comment:31 Changed 11 months ago by git

  • Commit changed from 598b0c233a501dba64b66c26577d533c826d9ad5 to d65299c666b9ed71a8f93cfb0b830c15d9e923f8

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

3cf5ee4Merge commit 'a50ddf88975086b14a49895e371477df00fd57b5' of git://trac.sagemath.org/sage into t/30940/src_bin_sage_list_packages__make_it_work_if_sage_root_is_not_available
b4927e1sage.misc.package: Remove/adjust non-robust doctests
e5fe752Merge tag '9.3.beta4' into t/30940/src_bin_sage_list_packages__make_it_work_if_sage_root_is_not_available
78ff9d5src/sage/misc/package.py: Add one more # optional - build
a44042fMerge branch 't/30940/src_bin_sage_list_packages__make_it_work_if_sage_root_is_not_available' into t/29124/script-packages-prereq-toolchain-bootstrap
64bde5fMerge tag '9.3.beta5' into t/30940/src_bin_sage_list_packages__make_it_work_if_sage_root_is_not_available
e9a7572src/sage/misc/package.py: Improve source formatting
c7bcda9Merge branch 't/30940/src_bin_sage_list_packages__make_it_work_if_sage_root_is_not_available' into t/29124/script-packages-prereq-toolchain-bootstrap
9988c5fci-cygwin*.yml: Adjust to new script packages _bootstrap, _prereq
d65299cMerge branch 't/29124/script-packages-prereq-toolchain-bootstrap' into t/31064/ci_cygwin__yml__adjust_to_new_script_packages__bootstrap___prereq

comment:32 Changed 11 months ago by mkoeppe

  • Description modified (diff)
  • Summary changed from ci-cygwin*.yml: Adjust to new script packages _bootstrap, _prereq, delegate to tox to ci-cygwin*.yml: delegate to tox, add more stages, use more specific SAGE_LOCAL

comment:33 Changed 11 months ago by git

  • Commit changed from d65299c666b9ed71a8f93cfb0b830c15d9e923f8 to a5e405116fa41c9afb3c5f46fbde2489616d267f

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

ab19133Merge branch 't/29124/script-packages-prereq-toolchain-bootstrap' into t/30944/tox__improve_local_sudo_ubuntu_standard
a5e4051Merge branch 't/30944/tox__improve_local_sudo_ubuntu_standard' into t/31064/ci_cygwin__yml__adjust_to_new_script_packages__bootstrap___prereq

comment:34 Changed 11 months ago by mkoeppe

Follow-up: #31211

comment:35 follow-ups: Changed 11 months ago by gh-tobiasdiez

  • Status changed from needs_review to needs_work

A few remarks after skimming the changes:

+ PACKAGES="python38 python38-pip"

I think it's better to install python using the python action, which brings a few goodies along: https://github.com/marketplace/actions/setup-python

+ C:\tools\cygwin\bin\bash -l -x -c

Can you try to put this as shell: C:\\tools\\cygwin\\bin\\bash -l -x -c {0}? Would make the workflow easier to read.

And then my usual rant about using tox for this ;-). I know you are a big fan of using tox, and this is fine. But I still think it's not the best tool for github workflows. You are cramping a lot of things into one command, which makes it hard to see at which stake the workflow fails. I would propose to add helper scripts so that the workflow looks like

  • Install dependencies: choco install (build/bin/get-dependencies cygwin) --source cygwin
  • Build: build/bin/build-local (with the script that is currently in tox: local, running bootstrap, configure and make)
  • Test: tox (running all python tests)

The scripts can of course be reused in tox.ini still giving you the chance to locally use tox for this (although I find it counterintuitive that tox might change my system installation).

comment:36 in reply to: ↑ 35 Changed 11 months ago by mkoeppe

Replying to gh-tobiasdiez:

+ C:\tools\cygwin\bin\bash -l -x -c

Can you try to put this as shell: C:\\tools\\cygwin\\bin\\bash -l -x -c {0}? Would make the workflow easier to read.

I'm pretty sure I went through several iterations, including use of the shell directive, before settling on something that actually works

comment:37 in reply to: ↑ 35 Changed 11 months ago by mkoeppe

Replying to gh-tobiasdiez:

You are cramping a lot of things into one command, which makes it hard to see at which stake the workflow fails.

This has not been a problem in my experience.

comment:38 Changed 11 months ago by mkoeppe

Let me explain again why I like to use tox to drive as much as possible in our CI. tox is a stable tool that has been under long term maintenance. For context, before we (well, I) started to use GH Actions, we (well, individual developers many of whom no longer have sufficient time) used buildbot, Circle CI, Travis CI, GitLab pipelines, ... Because our project is surfing on "free compute", the available technologies are changing faster than our project can sustain. Therefore it is important not to spend too much effort on development specific to the CI platform.

The present ticket consolidates GH Actions-specific code for installing Cygwin to just become part of the tox workflow. It is not enough to just factor through scripts that "could" also be run by developers on a local Windows machine: We currently do not even have any developers who do that. By using tox inside the GH Actions workflow, we actually make sure that these scripts are usable (and remain usable when changes are made) outside of a GH Actions environment.

comment:39 in reply to: ↑ 35 Changed 11 months ago by mkoeppe

Replying to gh-tobiasdiez:

A few remarks after skimming the changes:

+ PACKAGES="python38 python38-pip"

I think it's better to install python using the python action, which brings a few goodies along: https://github.com/marketplace/actions/setup-python

This is used in choco install $PACKAGES --source cygwin to install Cygwin and then these package in Cygwin.

I don't think the setup-python action can do that.

comment:40 follow-ups: Changed 11 months ago by gh-tobiasdiez

Thanks for the explanation. That's indeed something I understand and appreciate. My only problem is that managing the user system locally using tox is not very handy in my experience.

Here is an example I was experiencing the last few hours: The tox run encountered problems in the docbuild stage. The reason was/is a that ecl under wsl 1 has some problems and I needed to play around installing/deinstalling it combined with other changes. However, I couldn't use the tox command that the GH workflow uses since this installs ecl again. What I ended up doing was to copy & paste the commands printed by the tox run on github. That worked to some extend, but was a rather awful experience.

I think it is good to have an easy way for developers to investigate problems in the GH workflow on their local system. And for this, it is in my opinion crucial that they can quickly recreate the state that is used. In my opinion a clearer separation in system-setup, build and test is helpful in this regard. If this can be done using tox, sure why not.

comment:41 in reply to: ↑ 40 Changed 11 months ago by mkoeppe

Replying to gh-tobiasdiez:

managing the user system locally using tox is not very handy in my experience.

Right. Managing the global installation of Cygwin is not the final form of this development. What I would actually prefer (but have not had time to figure out -- any help is welcome) is to have the local-cygwin tox environment make an isolated installation of Cygwin. (I do this for the local-conda and local-homebrew environments.)

comment:42 in reply to: ↑ 40 Changed 11 months ago by mkoeppe

Replying to gh-tobiasdiez:

Here is an example I was experiencing the last few hours: The tox run encountered problems in the docbuild stage. The reason was/is a that ecl under wsl 1 has some problems and I needed to play around installing/deinstalling it combined with other changes. However, I couldn't use the tox command that the GH workflow uses since this installs ecl again. What I ended up doing was to copy & paste the commands printed by the tox run on github. That worked to some extend, but was a rather awful experience.

OK, I have run into this situation too. I think what is needed for the local-... environments is:

  • an environment variable that can be passed to tox to disable system package installs
  • a target or environment variable to give an interactive shell.

I will open a ticket for this.

Last edited 11 months ago by mkoeppe (previous) (diff)

comment:43 follow-up: Changed 11 months ago by gh-tobiasdiez

That would also work, thanks! Please also give a thought to my suggestion to extract some of the scripts in tox.ini to scripts that can be called independently of tox.

Last edited 11 months ago by gh-tobiasdiez (previous) (diff)

comment:44 Changed 11 months ago by mkoeppe

This is now #31216

comment:45 Changed 11 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:46 in reply to: ↑ 43 ; follow-up: Changed 11 months ago by mkoeppe

Replying to gh-tobiasdiez:

Please also give a thought to my suggestion to extract some of the scripts in tox.ini to scripts that can be called independently of tox.

I see two directions of improvement here:

  • Moving stuff from tox.ini (for tox -e local-...) and build/bin/write-dockerfile.sh (for tox -e docker-...) into our top-level Makefile. For example, the complicated rule for the commands of local consists entirely of workarounds for things that should really be fixed/improved there.
  • Improving the system package information scripts such as sage-print-system-package-command further.

Meta-ticket #29146 keeps track of such improvements, let's continue there.

On the other hand, I would be reluctant to add more scripts that developers can use to set up their system automatically. As I have written elsewhere, I think the current approach of printing command lines to educate developers about how to do things on their system is better for the Sage community than trying to provide scripts that do things automagically.

Last edited 11 months ago by mkoeppe (previous) (diff)

comment:47 in reply to: ↑ 46 Changed 11 months ago by gh-tobiasdiez

Replying to mkoeppe:

I see two directions of improvement here: Meta-ticket #29146 keeps track of such improvements, let's continue there.

Sounds good to me! Sorry for starting this somewhat off-topic discussion, but I very much appreciate your detailed explanations. It's always good to learn more about sage's inner workings ;-). I'll review this ticket as soon as the dependencies are merged (as it's hard to see right now which changes are coming from this ticket).

comment:48 Changed 11 months ago by mkoeppe

Great, and thanks for the discussion.

comment:49 Changed 10 months ago by dimpase

  • Reviewers changed from https://github.com/mkoeppe/sage/actions/runs/434342215 to Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm

comment:50 Changed 10 months ago by mkoeppe

Thank you!

comment:51 Changed 10 months ago by git

  • Commit changed from a5e405116fa41c9afb3c5f46fbde2489616d267f to 42f4458e3ae857f82759b3c9490ab34306a15f4d
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

42f4458Merge tag '9.3.beta7' into t/31064/ci_cygwin__yml__adjust_to_new_script_packages__bootstrap___prereq

comment:52 Changed 10 months ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:53 Changed 9 months ago by vbraun

  • Branch changed from u/mkoeppe/ci_cygwin__yml__adjust_to_new_script_packages__bootstrap___prereq to 42f4458e3ae857f82759b3c9490ab34306a15f4d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.