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

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

Closed
mkoeppe opened this issue Dec 16, 2020 · 68 comments
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 16, 2020

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.

Depends on #29124
Depends on #30944
Depends on #31062
Depends on #31084

CC: @seblabbe @kliem @antonio-rojas @embray @slel

Component: porting: Cygwin

Author: Matthias Koeppe

Branch/Commit: 42f4458

Reviewer: Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Dec 16, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 16, 2020

Changed dependencies from #29124 to #29124, #30944

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 17, 2020

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 17, 2020

Commit: b2ae68a

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 17, 2020

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 17, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 17, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 17, 2020

Work Issues: SAGE_LOCAL

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2020

Changed commit from b2ae68a to 11cf8db

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2020

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

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 18, 2020

Changed work issues from SAGE_LOCAL to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 18, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 18, 2020

Work Issues: update extract-sage-local.sh

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2020

Changed commit from 11cf8db to 05d57c0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2020

Changed commit from 05d57c0 to d87e160

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2020

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

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 18, 2020

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 19, 2020

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 9, 2021

comment:36

Replying to @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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 9, 2021

comment:37

Replying to @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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 9, 2021

comment:38

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 9, 2021

comment:39

Replying to @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.

@tobiasdiez
Copy link
Contributor

comment:40

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 9, 2021

comment:41

Replying to @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.)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 9, 2021

comment:42

Replying to @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.

@tobiasdiez
Copy link
Contributor

comment:43

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 9, 2021

comment:44

This is now #31216

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 10, 2021

comment:46

Replying to @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 Meta-ticket: Refactor and improve system package related scripts, tox.ini, build/bin/write_dockerfile.sh #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.

@tobiasdiez
Copy link
Contributor

comment:47

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).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 10, 2021

comment:48

Great, and thanks for the discussion.

@dimpase
Copy link
Member

dimpase commented Feb 2, 2021

Changed reviewer from https://github.com/mkoeppe/sage/actions/runs/434342215 to Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Feb 2, 2021

comment:49

lgtm

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 2, 2021

comment:50

Thank you!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2021

Changed commit from a5e4051 to 42f4458

@vbraun
Copy link
Member

vbraun commented Feb 20, 2021

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