Opened 2 years ago

Closed 2 years ago

#29401 closed enhancement (fixed)

Add documentation of tox and GitHub actions workflow to developer's manual

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.1
Component: documentation Keywords:
Cc: dimpase, jhpalmieri, vdelecroix, tscrim, mjo, fbissey, slelievre Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 99aca39 (Commits, GitHub, GitLab) Commit: 99aca39dea0226372a3f12aed3a13301a3380ff4
Dependencies: Stopgaps:

Status badges

Description


Attachments (1)

moar.diff (3.6 KB) - added by dimpase 2 years ago.
reviewer patch 2

Download all attachments as: .zip

Change History (58)

comment:1 Changed 2 years ago by mkoeppe

  • Branch set to u/mkoeppe/add_documentation_of_tox_and_github_actions_workflow_to_developer_s_manual

comment:2 Changed 2 years ago by git

  • Commit set to 5061729a8fa367518ab7b0a320f99fadae097e1c

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

5061729add info on GitHub Actions runners

comment:3 Changed 2 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc vdelecroix added
  • Status changed from new to needs_review

comment:4 Changed 2 years ago by dimpase

  • Status changed from needs_review to needs_work

doc does not build.

in portability_testing.rst warning: Could not lex literal_block as "shell-session".

perhaps it needs tagging as yaml, somehow

comment:5 Changed 2 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_work to needs_review
  • src/doc/en/developer/portability_testing.rst

    a b Testing on multiple platforms 
    99=============================
    1010
    1111Sage is intended to build and run on a variety of platforms,
    12 including all major Linux distributions, as well as macOS, and
     12including all major Linux distributions, as well as MacOS, and
    1313Windows (with Cygwin).
    1414
    1515There is considerable variation between these platforms.
    Using Sage's database of distribution prerequisites 
    142142
    143143The source code of the Sage distribution contains a database of
    144144package names in various distributions' package managers.  For
    145 example, the file ``build/pkgs/debian.txt`` contains the following::
     145example, the file ``build/pkgs/debian.txt`` contains the following
     146
     147.. code-block:: yaml
    146148
    147149  # This file, build/pkgs/debian.txt, contains names of Debian/Ubuntu packages
    148150  # needed for installation of Sage from source.

please apply this patch - then you can set it to positive review.

comment:6 Changed 2 years ago by git

  • Commit changed from 5061729a8fa367518ab7b0a320f99fadae097e1c to a4ce2d47440a1e4dd0dd76fa89bd74965cd9671f

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

a4ce2d4Reviewer patch

comment:7 Changed 2 years ago by dimpase

I'd add a link to https://wiki.sagemath.org/patchbot in the Sage patchbots section, and there are more typos and formatting issues. Let me post another patch...

Changed 2 years ago by dimpase

reviewer patch 2

comment:8 Changed 2 years ago by dimpase

please apply this, too

comment:9 Changed 2 years ago by git

  • Commit changed from a4ce2d47440a1e4dd0dd76fa89bd74965cd9671f to 71bf833441ee1a833954b51fda4048f9b664db4e

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

71bf833Reviewer patch

comment:10 Changed 2 years ago by mkoeppe

Thank you!

comment:11 Changed 2 years ago by mkoeppe

  • Cc tscrim added

comment:12 Changed 2 years ago by mkoeppe

  • Cc mjo added

comment:13 Changed 2 years ago by jhpalmieri

Trivial request:

  • src/doc/en/developer/portability_testing.rst

    diff --git a/src/doc/en/developer/portability_testing.rst b/src/doc/en/developer/portability_testing.rst
    index 20737e3ce1..16ea65340c 100644
    a b Sage is intended to build and run on a variety of platforms, 
    1212including all major Linux distributions, as well as MacOS, and
    1313Windows (with Cygwin).
    1414
    15 There is considerable variation between these platforms.
     15There is considerable variation among these platforms.
    1616To ensure that Sage continues to build correctly on users'
    1717machines, it is crucial to test changes to Sage, in particular
    1818when external packages are added or upgraded, on a wide

Some people may also complain that some of the lines are longer than 80 characters. I don't care much about this.

I just tried out the instructions for Docker. When I was just starting, apt-get install ... wasn't working: I needed to run apt-get update first (as in https://stackoverflow.com/questions/27273412/cannot-install-packages-inside-docker-ubuntu-image). Should this command be added?

In the "Generating Dockerfiles" section: am I understanding that you don't need to create a new git worktree with this approach? That's not completely clear as written.

comment:14 Changed 2 years ago by jhpalmieri

Also in the "Generating Dockerfiles" section: I tried

$ build/bin/write-dockerfile.sh debian "@(standard|optional)" > ../Dockerfile
$ docker build . -f ../Dockerfile ...

so as not to pollute my Sage directory with untracked files, but it didn't work:

ERRO[0000] Can't add file /Users/palmieri/Desktop/Sage_stuff/git/sage/.git/TEMP/rebase-apply/0001 to tar: io: read/write on closed pipe 
ERRO[0000] Can't close tar writer: io: read/write on closed pipe 
Sending build context to Docker daemon  37.89kB
error during connect: Post http://%2Fvar%2Frun%2Fdocker.sock/v1.40/build?buildargs=%7B%22BASE_IMAGE%22%3A%22ubuntu-latest-sage%22%2C%22NUMPROC%22%3A%224%22%7D&cachefrom=%5B%5D&cgroupparent=&cpuperiod=0&cpuquota=0&cpusetcpus=&cpusetmems=&cpushares=0&dockerfile=.dockerfile.13cd21991950b4666007&labels=%7B%7D&memory=0&memswap=0&networkmode=default&rm=1&shmsize=0&target=&ulimits=null&version=1: archive/tar: write too long

Giving an absolute path didn't help.

Also, once I get this going, where does the build take place? That is, can I look at the log files without running a second iteration of docker?

comment:15 Changed 2 years ago by mkoeppe

Docker contexts are tricky!

comment:16 Changed 2 years ago by mkoeppe

It's easiest to put the Dockerfile somewhere in the tree.

comment:17 Changed 2 years ago by mkoeppe

Thanks for the feedback. I'll revise the writing to address your questions

comment:18 Changed 2 years ago by git

  • Commit changed from 71bf833441ee1a833954b51fda4048f9b664db4e to f7954db62e65d458742936e2f2b0a0f2ec6742e6

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

f7954dbReviewer patch

comment:19 Changed 2 years ago by git

  • Commit changed from f7954db62e65d458742936e2f2b0a0f2ec6742e6 to 1e16fc14f651ad6288de1e17d4c379910c369cc3

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

8322a40src/doc/en/developer/portability_testing.rst: Mention apt-get update
1e16fc1Add more details

comment:20 Changed 2 years ago by mkoeppe

Ready for reading

comment:21 Changed 2 years ago by git

  • Commit changed from 1e16fc14f651ad6288de1e17d4c379910c369cc3 to 57236ce5787ca2cd8f9c43b01267541038ed24a1

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

57236cereformatting

comment:22 Changed 2 years ago by git

  • Commit changed from 57236ce5787ca2cd8f9c43b01267541038ed24a1 to b70226af5c36d989dc2f826e0a783af3f16118a7

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

b70226aAdd documentation on tox -e local

comment:23 Changed 2 years ago by git

  • Commit changed from b70226af5c36d989dc2f826e0a783af3f16118a7 to 229a090e3e35301c67837bffec71ad1dae610b68

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

229a090Add documentation of local-homebrew-macos

comment:24 follow-up: Changed 2 years ago by dimpase

please mention that docker examples in the manual are run on MacOS.


New commits:

229a090Add documentation of local-homebrew-macos

comment:25 Changed 2 years ago by git

  • Commit changed from 229a090e3e35301c67837bffec71ad1dae610b68 to 666043c49fb2ac540e334cb9ade5606a2f00239b

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

666043cmore on GitHub Actions

comment:26 follow-up: Changed 2 years ago by jhpalmieri

I'm curious: at the beginning, why do make configure before starting docker?

comment:27 Changed 2 years ago by git

  • Commit changed from 666043c49fb2ac540e334cb9ade5606a2f00239b to 752a9a55c42a6ff5b098d2f0aa7133e39f7fb9df

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

752a9a5More explanation regarding mac / linux

comment:28 in reply to: ↑ 26 Changed 2 years ago by mkoeppe

Replying to jhpalmieri:

I'm curious: at the beginning, why do make configure before starting docker?

In the initial examples I am avoiding a discussion of bootstrapping on the container -- because this needs more packages

comment:29 in reply to: ↑ 24 Changed 2 years ago by mkoeppe

Replying to dimpase:

please mention that docker examples in the manual are run on MacOS.

Done, please take a look

comment:30 Changed 2 years ago by mkoeppe

  • Cc fbissey added

comment:31 Changed 2 years ago by mkoeppe

  • Cc slelievre added

comment:32 Changed 2 years ago by dimpase

Lists of packages to install are also in Installation manual. Please at least mention this (ideally, add a cross-link)

comment:33 Changed 2 years ago by dimpase

docker run does not quite work as shown (I'm trying this on a Debian host). I have

$ docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
ubuntylatest        latest              ee59e71b3721        13 minutes ago      1.53GB

but

$ docker run -it   --mount type=bind,source=$(pwd),target=/sage ubuntuminimal
Unable to find image 'ubuntuminimal:latest' locally
docker: Error response from daemon: pull access denied for ubuntuminimal, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.

I have to use containter ID:

$ docker run -it   --mount type=bind,source=$(pwd),target=/sage ee59e71b3721
root@6b44c73a10bb:/# 

comment:34 follow-up: Changed 2 years ago by mkoeppe

You have a typo there: ubuntylatest vs. ubuntuminimal

comment:35 in reply to: ↑ 34 Changed 2 years ago by dimpase

Replying to mkoeppe:

You have a typo there: ubuntylatest vs. ubuntuminimal

oops, indeed, sorry for noise.

comment:36 Changed 2 years ago by dimpase

perhaps, add a link to https://docs.docker.com/get-started/part3/ where you talk about sharing of images, and just send people over to "docker docs"

comment:37 Changed 2 years ago by dimpase

please add few lines about tox in general before Symbolic names of system configurations, otherwise the jump into tox details is way too steep.

comment:38 Changed 2 years ago by git

  • Commit changed from 752a9a55c42a6ff5b098d2f0aa7133e39f7fb9df to 7e96a4fad549c6b339cdade01fae08e50ab9ce47

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

0c36e96add links to docker docs
9f061edadd crossref to Installation Guide
ed4b6dbmerge the sections on tox factors
5c21dfaelide some details from terminal output
8d729b8change 'ls -l' to 'ls -la' and elide some details
7e96a4fElide more details, shorten directory names more

comment:39 Changed 2 years ago by dimpase

typos, please apply

  • src/doc/en/developer/portability_testing.rst

    a b The image ``ubuntu-latest-minimal-17`` can be run in as many 
    293293containers as we want and can also be shared with other users or
    294294developers so that they can run it in a container on their machine.
    295295(See the Docker documentation on how to `share images on Docker Hub
    296 <https://docs.docker.com/get-started/part3/>` or to `save images to a
     296<https://docs.docker.com/get-started/part3/>`_ or to `save images to a
    297297tar archive
    298 <https://docs.docker.com/engine/reference/commandline/save/>`.)
     298<https://docs.docker.com/engine/reference/commandline/save/>`_.)
    299299
    300300This facilitates collaboration on fixing portability bugs of the Sage
    301301distribution.  After reproducing a portability bug on a container,

comment:40 Changed 2 years ago by git

  • Commit changed from 7e96a4fad549c6b339cdade01fae08e50ab9ce47 to 778a5924acf171db9010954065e2f5f273d9607f

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

778a592Reviewer patch

comment:41 Changed 2 years ago by dimpase

more missing _ (hopefully all now):

  • src/doc/en/developer/portability_testing.rst

    a b using a Docker client on your Linux, Mac, or Windows box, as well as 
    5151on various cloud services.
    5252
    5353To get started, you need to install a `Docker client
    54 <https://docs.docker.com/install/>`.  The clients are available for
     54<https://docs.docker.com/install/>`_.  The clients are available for
    5555Linux, Mac, and Windows.  The clients for the latter are known as
    5656"Docker Desktop".
    5757
    5858All examples in this section were obtained using Docker Desktop for
    5959Mac; but the `command-line user interface
    60 <https://docs.docker.com/engine/reference/commandline/cli/>` for the
     60<https://docs.docker.com/engine/reference/commandline/cli/>`_ for the
    6161other platforms is identical.
    6262
    6363All major Linux distributions provide ready-to-use Docker images,
    automating testing, Sage defines symbolic names composed of several 
    574574Automatic Docker-based build testing using tox
    575575----------------------------------------------
    576576
    577 `tox <https://tox.readthedocs.io/en/latest/>` is a Python package that
     577`tox <https://tox.readthedocs.io/en/latest/>`_ is a Python package that
    578578is widely used for automating tests of Python projects.
    579579
    580580Install ``tox`` for use with your system Python, for example using::

comment:42 Changed 2 years ago by git

  • Commit changed from 778a5924acf171db9010954065e2f5f273d9607f to 86981268d25044ceb8663d9453b92ef2eefe11a3

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

8698126Reviewer patch

comment:43 Changed 2 years ago by mkoeppe

Thank you!

comment:44 Changed 2 years ago by mkoeppe

Any more corrections or suggestions for this new section in the manual?

comment:45 Changed 2 years ago by git

  • Commit changed from 86981268d25044ceb8663d9453b92ef2eefe11a3 to b7b45fe9a62a533db9f4199da5a57e0afd4d4139

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

7eac94amore on GitHub Actions
426aec1More explanation regarding mac / linux
43195f6add links to docker docs
3d2c3faadd crossref to Installation Guide
6153945merge the sections on tox factors
ab11ebdelide some details from terminal output
c7fcf91change 'ls -l' to 'ls -la' and elide some details
e73bc8dElide more details, shorten directory names more
a31b2a0Reviewer patch
b7b45feReviewer patch

comment:46 Changed 2 years ago by mkoeppe

Rebased on 9.1.beta9, needs review

comment:47 Changed 2 years ago by dimpase

unfinished edit here at line 570, ending with several

comment:48 Changed 2 years ago by git

  • Commit changed from b7b45fe9a62a533db9f4199da5a57e0afd4d4139 to f7d2dc9fd3975cba376e246797d7cee2b47a3af1

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

f7d2dc9Remove old section head

comment:49 Changed 2 years ago by dimpase

make this a proper list:

  • src/doc/en/developer/portability_testing.rst

    a b workflow have finished. Each job generates one tarball. 
    854854during the build.
    855855
    856856The following procedure seems to work well for testing branches during
    857 development.  Create a branch from a recent beta release that contains
    858 the default GitHub Actions configuration; name it ``TESTER``, say.
    859 Edit ``$SAGE_ROOT/.github/workflows/tox.yml`` to include the system
    860 configurations that you wish to test.  Commit and push the branch to
    861 your GitHub fork of sage.  Next, push your development branch to your
    862 GitHub repository and create a pull request against the ``TESTER``
    863 branch.  This will trigger the GitHub Actions workflow.
     857development:
     858
     859- Create a branch from a recent beta release that contains the default GitHub Actions configuration; name it ``TESTER``, say.
     860- Edit ``$SAGE_ROOT/.github/workflows/tox.yml`` to include the system configurations that you wish to test.
     861- Commit and push the branch to your GitHub fork of sage.
     862- Push your development branch to your GitHub repository and create a pull request against the ``TESTER`` branch. This will trigger the GitHub Actions workflow.

also, there are few lines in the file after this, which do not go into the output. Perhaps, just remove them.

comment:50 Changed 2 years ago by git

  • Commit changed from f7d2dc9fd3975cba376e246797d7cee2b47a3af1 to 99aca39dea0226372a3f12aed3a13301a3380ff4

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

99aca39Reformat list, remove unfinished commented out section

comment:51 Changed 2 years ago by dimpase

  • Status changed from needs_review to positive_review

OK. Squash these many commits, I guess, too.

comment:52 Changed 2 years ago by git

  • Commit changed from 99aca39dea0226372a3f12aed3a13301a3380ff4 to aba215d7d20437db5eaaf21c32543e117f0713f4
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

aba215dsrc/doc/en/developer/portability_testing.rst: New chapter. (squashed)

comment:53 Changed 2 years ago by mkoeppe

  • Status changed from needs_review to positive_review

Thank you!

comment:54 Changed 2 years ago by mkoeppe

Looks like @vbraun has merged the unsquashed branch already. Resetting to that.

comment:55 Changed 2 years ago by git

  • Commit changed from aba215d7d20437db5eaaf21c32543e117f0713f4 to 99aca39dea0226372a3f12aed3a13301a3380ff4
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. Last 10 new commits:

43195f6add links to docker docs
3d2c3faadd crossref to Installation Guide
6153945merge the sections on tox factors
ab11ebdelide some details from terminal output
c7fcf91change 'ls -l' to 'ls -la' and elide some details
e73bc8dElide more details, shorten directory names more
a31b2a0Reviewer patch
b7b45feReviewer patch
f7d2dc9Remove old section head
99aca39Reformat list, remove unfinished commented out section

comment:56 Changed 2 years ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:57 Changed 2 years ago by vbraun

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