Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#30865 closed enhancement (fixed)

sage_bootstrap: Update/extend "sage -package", fix "make download"

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.3
Component: build: configure Keywords:
Cc: slabbe, gh-tobiasdiez, slelievre, jhpalmieri, vbraun Merged in:
Authors: Matthias Koeppe Reviewers: Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: f33c363 (Commits, GitHub, GitLab) Commit:
Dependencies: #30648 Stopgaps:

Status badges

Description (last modified by mkoeppe)

(from #29146, #30861)

There are several places where the spkg (build/pkgs/*/) and system package information (build/pkgs/*/distros/*.txt) is parsed:

  • bootstrap
  • src/doc/bootstrap
  • build/bin/write_dockerfile.sh
  • build/bin/sage-get-system-packages
  • tox.ini (for local-homebrew, local-conda)
  • .github/workflows/ci-wsl.yml (using powershell)
  • src/bin/sage-download-upstream
  • m4/sage_spkg_*.m4

The purpose of this ticket is to prepare refactoring of this code by extending the sage_bootstrap package.

  • We update sage_bootstrap.package so that it understands non-normal packages (i.e., script and pip packages, which do not have checksums.ini)
  • We change sage -package list so it supports the following invocations:
    • sage -package list :standard: :optional: (several "classes" (types), not just one)
    • sage -package list --has-file=spkg-configure.m4 :standard: (needed for bootstrap, src/doc/bootstrap)
    • sage -package list --has-file=spkg-configure.m4 --has-file=distros/debian.txt :standard: :optional: (needed for build/bin/write-dockerfile.sh, tox.ini)
    • sage -package list --has-file=SPKG.rst :all: (needed for src/doc/bootstrap)
  • We fix and extend sage -package download (follow up from #30648) so it supports
    • sage -package download :all: (again - was broken)
    • sage -package download --allow-upstream :all: (allows retrieving from upstream_url) and use it for make download (#29896), removing src/bin/sage-download-upstream

To run the testsuite of sage_bootstrap, use

  (cd build && tox)

Follow-up:

  • The actual refactoring of the above scripts (#30947, #30951, #30968, ...)
  • Wishlist item: sage -package list --not-source=pip :optional: :experimental: (would simplify bootstrap)

Change History (46)

comment:1 Changed 2 years ago by mkoeppe

Developers interested in improving the system package advice may want to work on this ticket

comment:2 Changed 2 years ago by mkoeppe

Authors: Matthias Koeppe
Description: modified (diff)
Summary: sage_bootstrap: Implement system package toolssage_bootstrap: Update/extend system package tools

comment:3 Changed 2 years ago by mkoeppe

Branch: u/mkoeppe/sage_bootstrap__update_extend_system_package_tools

comment:4 Changed 2 years ago by mkoeppe

Cc: vbraun added
Commit: c405d1026b35e181f114ebffb4b4452a0b22b75b
Description: modified (diff)

New commits:

2265048sage_bootstrap.package: Handle non-normal packages (which have no checksums.ini etc.)
c405d10sage_bootstrap.package.Package._init_type: Do not accept 'script' or 'pip' any more

comment:5 Changed 2 years ago by mkoeppe

Dependencies: #30648

comment:6 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:7 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:8 Changed 2 years ago by git

Commit: c405d1026b35e181f114ebffb4b4452a0b22b75b3f9131304c9d0f93b76e6d85f1cbddb4be2e8c9c

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

e46351330648: download_cls -> download
0b343d1Merge commit 'e4635137337490ce452945728c8667f3f51b5b40' of git://trac.sagemath.org/sage into t/30865/sage_bootstrap__update_extend_system_package_tools
5fc8441sage_bootstrap.app.Application.download_cls: Pass allow_upstream to download
794a15csage_bootstrap.cndline: Back to using download_cls
6369bb4sage -package list: Handle --has-file, multiple package classes (types)
4fbf753src/doc/en/installation/source.rst: Remove outdated documentation on old-style packages
7082715Makefile: Update documentation of 'make download'
3f91313src/bin/sage-download-upstream: Remove, use 'sage --package download' instead

comment:9 Changed 2 years ago by git

Commit: 3f9131304c9d0f93b76e6d85f1cbddb4be2e8c9cf6b79bbc89bfe65a166dc0855ab62f93f90b7b88

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

69f7ea8sage_bootstrap.PackageClass: Accept several arguments, simplify use
40d6604sage_bootstrap.package.Package.has_file: New, use it
6475263sage_bootstrap.PackageClass: Handle filter has_files, simplify use
769f635sage_bootstrap.tarball: Raise an error if attempting to download a tarball of a non-normal package
f6b79bbsage_bootstrap.app.download_cls: Only attempt to download tarballs for normal packages (with checksums.ini)

comment:10 Changed 2 years ago by mkoeppe

Description: modified (diff)
Status: newneeds_review

comment:11 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:12 Changed 2 years ago by git

Commit: f6b79bbc89bfe65a166dc0855ab62f93f90b7b884be654b22e7961c615b8103bad5c1f8832e18482

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

4be654bsage_bootstrap.cmdline: Initialize has_files filter correctly

comment:13 Changed 2 years ago by mkoeppe

Description: modified (diff)

Ready for review

comment:14 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:15 Changed 2 years ago by mkoeppe

Summary: sage_bootstrap: Update/extend system package toolssage_bootstrap: Update/extend "sage -package", fix "make download"

comment:16 Changed 2 years ago by slabbe

The current branch is doing this:

-    def download_cls(self, package_name_or_class):
+    def download_cls(self, package_name_or_class, allow_upstream=False):

So should the change I made in #30648 be reverted then?

(I recall that the problem fixed in #30648 was that download_cls method had no argument allow_upstream where download method did, so I changed download_cls to download. But at the time, I was wondering if that was the good fix. Because another fix could have been to add the allow_upstream argument to the download_cls method which you are doing in this branch)

Last edited 2 years ago by slabbe (previous) (diff)

comment:17 Changed 2 years ago by slabbe

Doing += on lists creates a new list in memory as opposed to append or extend:

sage: L = list(range(10))                                                       
sage: K = list('abcd')                                                          
sage: id(L)                                                                     
140556349258880
sage: id(L+K)                                                                   
140556349147776
sage: L.extend(K)                                                               
sage: id(L)                                                                     
140556349258880

So using += to append elements to a list uses a quadratic amount of memory until the garbage collector gets called.

So I would suggest the following change:

- self.names += [package_name_or_class]
+ self.names.append(package_name_or_class)

as well as (5 times):

- self.names += [pkg.name for pkg in Package.all() if predicate(pkg)]
+ self.names.extend(pkg.name for pkg in Package.all() if predicate(pkg))
Last edited 2 years ago by slabbe (previous) (diff)

comment:18 Changed 2 years ago by slabbe

I will try the branch later today, and come back with further comments if I have.

comment:19 in reply to:  16 ; Changed 2 years ago by mkoeppe

Replying to slabbe:

The current branch is doing this:

-    def download_cls(self, package_name_or_class):
+    def download_cls(self, package_name_or_class, allow_upstream=False):

So should the change I made in #30648 be reverted then?

Yes, in fact I have already reverted it as part of the branch on this ticket.

comment:20 Changed 2 years ago by git

Commit: 4be654b22e7961c615b8103bad5c1f8832e18482b762c3dfee999746ad0b8507460178ee6a63966b

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

b762c3dsage_bootstrap.expand_class: Use .append, .extend instead of += for lists

comment:21 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:22 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:23 in reply to:  19 Changed 2 years ago by slabbe

Replying to mkoeppe:

Yes, in fact I have already reverted it as part of the branch on this ticket.

Ok, yes, I agree. Since #30648 is not in sage yet, this is why I don't see it in the diff.

comment:24 Changed 2 years ago by slabbe

Status: needs_reviewneeds_work

(cd build && tox)

I did that. It finishes with a big list of typos and says that there were errors while running the tests. When I scroll back, I can't see the errors, because the typos takes all of the lines in the finite history of the terminal. Are the output of tox logged somewhere?

I find it weird to call it class of package and use cls (which is often use for representing a Python class. Maybe nature of the package? Anyway, this is out of topic.

(0) I confirm that sage --package list works with inputs such as --has-file=spkg-configure.m4, --has-file=distros/debian.txt, --has-file=SPKG.rst. Also sage --package download works. Also make download works.

Sorry, I still have few more comments listed below.

(1) When I read the documentation of sage --package, I don't know what Sage Bootstrap Library means. I suggest to add a sentence in the file sage_bootstrap.cmdline.py to explain it. Something as follows, but please replace the verb deal with any more precise verb:

 description = \
 """
 Sage Bootstrap Library
+
+It deals with all of the packages in SageMath.
 """

(2) I don't like the following error message:

$ sage --package list :standardd:
Traceback (most recent call last):
  File "/home/slabbe/GitBox/sage/build/bin/sage-package", line 42, in <module>
    run()
  File "/home/slabbe/GitBox/sage/build/bin/../sage_bootstrap/cmdline.py", line 306, in run
    app.list_cls(*args.package_class, has_files=args.has_files)
  File "/home/slabbe/GitBox/sage/build/bin/../sage_bootstrap/app.py", line 69, in list_cls
    pc = PackageClass(*package_classes, **filters)
  File "/home/slabbe/GitBox/sage/build/bin/../sage_bootstrap/expand_class.py", line 48, in __init__
    raise ValueError('Package name cannot start with ":", got %s', package_name_or_class)
ValueError: ('Package name cannot start with ":", got %s', ':standardd:')

It tells me the input cannot start with ":", but this is false, as :standard: is accepted.

Thus, I would suggest the following change in sage_bootstrap/expand_class.py:

             else:
-                if package_name_or_class.startswith(':'):
-                    raise ValueError('Package name cannot start with ":", got %s', package_name_or_class)
-                if package_name_or_class.endswith(':'):
-                    raise ValueError('Package name cannot end with ":", got %s', package_name_or_class)
+                if package_name_or_class.startswith(':') or package_name_or_class.endswith(':'):
+                    raise ValueError('Valid package classes are :all:, :standard:, :optional:, :experimental: or :huge:, got %s', package_name_or_class)
                 self.names.append(package_name_or_class)

Question: do you confirm that the following is a desirable feature?

$ sage --package list pari
pari

(3) Finally, in build/sage_bootstrap/package.py, I suggest:

-        if pattern:
-            return self.tarball_pattern.replace('VERSION', self.version)
+        if pattern:
+            return pattern.replace('VERSION', self.version)

(4) Question: What is the meaning of the expression non-normal in the file build/sage_bootstrap/tarball.py ?

I am currently waiting for the report of make ptestlong.

Last edited 2 years ago by slabbe (previous) (diff)

comment:25 Changed 2 years ago by slabbe

The make ptestlong finished with:

----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/interfaces/singular.py  # Killed due to segmentation fault
----------------------------------------------------------------------

which is unrelated to this ticket.

comment:26 in reply to:  25 Changed 2 years ago by slelievre

Replying to slabbe:

The make ptestlong finished with:

----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/interfaces/singular.py  # Killed due to segmentation fault
----------------------------------------------------------------------

which is unrelated to this ticket.

Indeed that is different, and tracked at #30945.

comment:27 Changed 2 years ago by slabbe

Reviewers: Sébastien Labbé

comment:28 in reply to:  24 ; Changed 2 years ago by mkoeppe

Replying to slabbe:

(cd build && tox)

I did that. It finishes with a big list of typos

If it shows typos, then you probably ran sage -tox, not tox. If you don't have tox in your global environment, you can use (cd build && sage -sh -c tox).

Are the output of tox logged somewhere?

Yes, it creates a subdirectory .tox, where you'll find logs.

comment:29 Changed 2 years ago by git

Commit: b762c3dfee999746ad0b8507460178ee6a63966b51db2a9e3bbbe730846c00831991a3e7b8ff5b31

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

51db2a9sage_bootstrap: Eliminate terminology 'package class' in help and error messages

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

Replying to slabbe:

I find it weird to call it class of package and use cls (which is often use for representing a Python class. Maybe nature of the package? Anyway, this is out of topic.

I agree. I have now made a change to eliminate this (unexplained) terminology from user-facing messages. But I haven't changed the class name PackageClass or argument names, to keep the diff small.

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

Replying to slabbe:

(4) Question: What is the meaning of the expression non-normal in the file build/sage_bootstrap/tarball.py ?

This is referring to https://doc.sagemath.org/html/en/developer/packaging.html#package-source-types

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

Replying to slabbe:

I would suggest the following change in sage_bootstrap/expand_class.py:

             else:
-                if package_name_or_class.startswith(':'):
-                    raise ValueError('Package name cannot start with ":", got %s', package_name_or_class)
-                if package_name_or_class.endswith(':'):
-                    raise ValueError('Package name cannot end with ":", got %s', package_name_or_class)
+                if package_name_or_class.startswith(':') or package_name_or_class.endswith(':'):
+                    raise ValueError('Valid package classes are :all:, :standard:, :optional:, :experimental: or :huge:, got %s', package_name_or_class)
                 self.names.append(package_name_or_class)

I have made a similar change in the above commit.

Question: do you confirm that the following is a desirable feature?

$ sage --package list pari
pari

Yes, this is a feature, and the above commit documents it.

comment:33 Changed 2 years ago by git

Commit: 51db2a9e3bbbe730846c00831991a3e7b8ff5b31c0a0329d842ad8745789f8bf61bc456f0473e1fe

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

c0a0329sage_bootstrap.cmdline: Expand description

comment:34 Changed 2 years ago by mkoeppe

Status: needs_workneeds_review

comment:35 in reply to:  28 ; Changed 2 years ago by slabbe

Replying to mkoeppe:

If it shows typos, then you probably ran sage -tox, not tox. If you don't have tox in your global environment, you can use (cd build && sage -sh -c tox).

Well, I first did sudo apt install tox on a Ubuntu 18.04 but, then I got problems because that tox is using python2. So, your guess is right, that was after the run of sage -tox.

Are the output of tox logged somewhere?

Yes, it creates a subdirectory .tox, where you'll find logs.

Great. Thank you.

comment:36 Changed 2 years ago by slabbe

Thanks for the answers and commits, I will check this tomorrow morning Bordeaux time.

[I think my comment (3) is still not fixed (well, it is just a small detail).]

comment:37 in reply to:  35 ; Changed 2 years ago by mkoeppe

Replying to slabbe:

I first did sudo apt install tox on a Ubuntu 18.04 but, then I got problems because that tox is using python2.

That should actually work too. sage_bootstrap is designed to work on a wide range of Python versions including 2.x. tox provisions virtual environments to test the package with each available Python version. To run it with a specific Python version, you can do tox -e py37 etc. It does not matter on what version of Python tox itself runs.

comment:38 Changed 2 years ago by git

Commit: c0a0329d842ad8745789f8bf61bc456f0473e1fef33c3635ba36440b260644c127dcc94e12c3ab40

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

f33c363sage_bootstrap.package.Package.tarball_pattern: Simplify code

comment:39 in reply to:  37 Changed 2 years ago by slabbe

Replying to mkoeppe:

Replying to slabbe:

I first did sudo apt install tox on a Ubuntu 18.04 but, then I got problems because that tox is using python2.

That should actually work too. sage_bootstrap is designed to work on a wide range of Python versions including 2.x. tox provisions virtual environments to test the package with each available Python version. To run it with a specific Python version, you can do tox -e py37 etc. It does not matter on what version of Python tox itself runs.

Ok, I see. But after cd build, the tox command prints few lines and seems to get stuck:

$ cd build
$ tox
GLOB sdist-make: /home/slabbe/GitBox/sage/build/setup.py
py26 create: /home/slabbe/GitBox/sage/build/.tox/py26
ERROR: InterpreterNotFound: python2.6
py27 create: /home/slabbe/GitBox/sage/build/.tox/py27
py27 inst: /home/slabbe/GitBox/sage/build/.tox/dist/sage_bootstrap-1.0.zip
py27 installed: DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 is no longer maintained. pip 21.0 will drop support for Python 2.7 in January 2021. More details about Python 2 support in pip can be found at https://pip.pypa.io/en/latest/development/release-process/#python-2-support pip 21.0 will remove support for this functionality.,pkg-resources==0.0.0,sage-bootstrap @ file:///home/slabbe/GitBox/sage/build/.tox/dist/sage_bootstrap-1.0.zip
py27 runtests: PYTHONHASHSEED='3230691582'
py27 runtests: commands[0] | python2.7 -m unittest discover
......

comment:40 Changed 2 years ago by slabbe

Actually, it did not get stuck, but I obtain three errors after few minutes:

$ tox
GLOB sdist-make: /home/slabbe/GitBox/sage/build/setup.py
py26 create: /home/slabbe/GitBox/sage/build/.tox/py26
ERROR: InterpreterNotFound: python2.6
py27 create: /home/slabbe/GitBox/sage/build/.tox/py27
py27 inst: /home/slabbe/GitBox/sage/build/.tox/dist/sage_bootstrap-1.0.zip
py27 installed: DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 is no longer maintained. pip 21.0 will drop support for Python 2.7 in January 2021. More details about Python 2 support in pip can be found at https://pip.pypa.io/en/latest/development/release-process/#python-2-support pip 21.0 will remove support for this functionality.,pkg-resources==0.0.0,sage-bootstrap @ file:///home/slabbe/GitBox/sage/build/.tox/dist/sage_bootstrap-1.0.zip
py27 runtests: PYTHONHASHSEED='3230691582'
py27 runtests: commands[0] | python2.7 -m unittest discover
......FF..............[xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx]
[......................................................................]
......F........ERROR [package|all:249]: Failed to open pari
..
======================================================================
FAIL: test_error (test.test_download.DownloadTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slabbe/GitBox/sage/build/test/test_download.py", line 49, in test_error
    self.assertIsNotFoundError(log.messages())
  File "/home/slabbe/GitBox/sage/build/test/test_download.py", line 64, in assertIsNotFoundError
    "[Errno 404] Not Found: '//files.sagemath.org/sage_bootstrap/this_url_does_not_exist'"))
AssertionError: False is not true

======================================================================
FAIL: test_ignore_errors (test.test_download.DownloadTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slabbe/GitBox/sage/build/test/test_download.py", line 57, in test_ignore_errors
    self.assertIsNotFoundError(log.messages())
  File "/home/slabbe/GitBox/sage/build/test/test_download.py", line 64, in assertIsNotFoundError
    "[Errno 404] Not Found: '//files.sagemath.org/sage_bootstrap/this_url_does_not_exist'"))
AssertionError: False is not true

======================================================================
FAIL: test_checksum (test.test_tarball.TarballTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slabbe/GitBox/sage/build/test/test_tarball.py", line 59, in test_checksum
    '[......................................................................]\n')
AssertionError: '[xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx]\n[......................................................................]\n' != '[......................................................................]\n'

----------------------------------------------------------------------
Ran 39 tests in 228.835s

FAILED (failures=3)

After that it seems tox does the same with other version of Python it finds on the system. With python 3.6, it returns two errors instead of three (the test_checksum does not fail).

Let me now check whether this is related to the current ticket or not by rerunning it after a git checkout develop.

comment:41 Changed 2 years ago by slabbe

I also get errors with 9.3.beta1, but not exactly the same. test_ignore_errors and test_error both fails as with the branch. But test_download fails only on 9.3.beta1 whereas test_checksum fails only with the current branch.

======================================================================
FAIL: test_error (test.test_download.DownloadTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slabbe/GitBox/sage/build/test/test_download.py", line 49, in test_error
    self.assertIsNotFoundError(log.messages())
  File "/home/slabbe/GitBox/sage/build/test/test_download.py", line 63, in assertIsNotFoundError
    self.assertTrue(messages[0][1].endswith(
AssertionError: False is not true

======================================================================
FAIL: test_ignore_errors (test.test_download.DownloadTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slabbe/GitBox/sage/build/test/test_download.py", line 57, in test_ignore_errors
    self.assertIsNotFoundError(log.messages())
  File "/home/slabbe/GitBox/sage/build/test/test_download.py", line 63, in assertIsNotFoundError
    self.assertTrue(messages[0][1].endswith(
AssertionError: False is not true

======================================================================
FAIL: test_download (test.test_package_cmdline.SagePackageTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slabbe/GitBox/sage/build/test/test_package_cmdline.py", line 115, in test_download
    self.assertTrue(stderr.startswith('Using cached file'))
AssertionError: False is not true

----------------------------------------------------------------------
Ran 39 tests in 134.424s

FAILED (failures=3)

I don't know how much this is related to the current ticket. From my point of view, I give a positive review to the commits done up to now.

Matthias, I let you change the status to positive review if you think the tox issues are unrelated or if they can be dealt in another ticket.

comment:42 Changed 2 years ago by mkoeppe

Status: needs_reviewpositive_review

Thank you!

(I also get the additional test_checksum failure on this branch. This test only passes on a release tag; this is not specific to this ticket. The other failures, also not specific to this ticket, should be investigated on a separate ticket.)

comment:43 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:44 Changed 2 years ago by vbraun

Branch: u/mkoeppe/sage_bootstrap__update_extend_system_package_toolsf33c3635ba36440b260644c127dcc94e12c3ab40
Resolution: fixed
Status: positive_reviewclosed

comment:45 Changed 2 years ago by slelievre

Commit: f33c3635ba36440b260644c127dcc94e12c3ab40

I tried make download and it failed to download scipoptsuite.

Which is expected. Indeed, reading from the last scipoptsuite ticket:

  • #24662: Upgrade scipoptsuite to 5.0.1

Upstream archive: <snip> (DO NOT put on sage servers -- we cannot redistribute this archive)

How do we deal with that?

comment:46 in reply to:  45 Changed 2 years ago by mkoeppe

Replying to slelievre:

(DO NOT put on sage servers -- we cannot redistribute this archive)

How do we deal with that?

Let's discuss this on #31034

Note: See TracTickets for help on using tickets.