#30940 closed enhancement (fixed)

src/bin/sage-list-packages: Make it work if SAGE_ROOT is not available

Reported by: mkoeppe Owned by:
Priority: critical Milestone: sage-9.3
Component: build Keywords: sd111
Cc: vdelecroix, gh-tobiasdiez, slabbe, thansen, jhpalmieri, chapoton, gh-kliem, slelievre Merged in:
Authors: Matthias Koeppe Reviewers: Tobias Diez, Jonathan Kliem
Report Upstream: N/A Work issues:
Branch: e9a7572 (Commits, GitHub, GitLab) Commit: e9a7572a37b228f4a74c35f680190e6b237f48d8
Dependencies: Stopgaps:

Status badges

Description

In this case we can still retrieve the installed versions from the installation records $SAGE_LOCAL/var/lib/sage/installed (in the case of the sage distribution), but we will not be able to provide all details such as remote_version.

Change History (33)

comment:1 Changed 18 months ago by mkoeppe

  • Branch set to u/mkoeppe/src_bin_sage_list_packages__make_it_work_if_sage_root_is_not_available

comment:2 Changed 18 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 3cc5edf85cb2c074ad12029637e09d799d7bde6f

New commits:

8ccc157sage.misc.package: Deprecate standard_packages, optional_packages, experimental_packages
c029fdfsage.misc.package.pip_installed_packages: Normalize package names
436cc51sage.misc.package.list_packages: Merge package lists from installation records and build/pkgs
b577ddcsage.misc.package.installed_packages: Omit .dummy and other dot files
3cc5edfsage.misc.package.is_package_installed: Remove outdated code

comment:3 Changed 18 months ago by mkoeppe

  • Cc vdelecroix added

comment:4 Changed 18 months ago by mkoeppe

  • Status changed from new to needs_review

comment:5 Changed 18 months ago by mkoeppe

  • Cc gh-tobiasdiez added

comment:6 follow-ups: Changed 18 months ago by gh-tobiasdiez

  • Reviewers set to Tobias Diez
  • Status changed from needs_review to needs_work

I think the previous code was easier to understand: if SAGE_PKGS is None, we don't need to go through the following for loop. But now one has to understand that the iteration is over an empty list. Better to exit the method early.

-    if not SAGE_PKGS:
-        return {}
-
-    try:
-        lp = os.listdir(SAGE_PKGS)
-    except FileNotFoundError:
-        return {}
+    if SAGE_PKGS:
+        try:
+            lp = os.listdir(SAGE_PKGS)
+        except FileNotFoundError:
+            pass
     for ...

Moreover, I was wondering if the name normalization is ok as it changes the output of a public facing method and thus might break user scripts.

Otherwise it looks good to me.

comment:7 in reply to: ↑ 6 Changed 18 months ago by mkoeppe

Replying to gh-tobiasdiez:

I was wondering if the name normalization is ok as it changes the output of a public facing method and thus might break user scripts.

Good point, I'll make this normalization optional

comment:8 Changed 18 months ago by git

  • Commit changed from 3cc5edf85cb2c074ad12029637e09d799d7bde6f to 7f80e3e91f176d1650dc2f3bbb7a25eef366e068

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

97942e3sage.misc.package.pip_installed_packages: Only normalize when optional arg normalization='spkg'
6706182sage.misc.package.list_packages: Return early if SAGE_PKGS is not available
9258849sage.misc.package.list_packages: Streamline handling of exclude_pip
7f80e3esage.misc.package.list_packages: Actually add new items to the list

comment:9 in reply to: ↑ 6 Changed 18 months ago by mkoeppe

  • Cc slabbe added
  • Status changed from needs_work to needs_review

Replying to gh-tobiasdiez:

I think the previous code was easier to understand: if SAGE_PKGS is None, we don't need to go through the following for loop. But now one has to understand that the iteration is over an empty list. Better to exit the method early.

Thanks, done.

comment:10 Changed 18 months ago by mkoeppe

  • Keywords sd111 added

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

comment:11 Changed 18 months ago by gh-tobiasdiez

  • Status changed from needs_review to positive_review

Code-wise it looks good to me now, thanks for the update!

comment:12 Changed 18 months ago by git

  • Commit changed from 7f80e3e91f176d1650dc2f3bbb7a25eef366e068 to 78bae1ff5356529af4246bc3d33162a6045dde37
  • 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:

78bae1fMerge tag '9.3.beta3' into t/30940/src_bin_sage_list_packages__make_it_work_if_sage_root_is_not_available

comment:13 Changed 18 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Merged current beta

comment:14 Changed 17 months ago by vbraun

  • Status changed from positive_review to needs_work
**********************************************************************
File "src/sage/misc/package.py", line 384, in sage.misc.package.is_package_installed
Failed example:
    for pkg in list_packages(pkg_sources=('pip'), local=True):  # optional - build
        assert not is_package_installed(pkg), "pip package is installed: {}".format(pkg)
Exception raised:
    Traceback (most recent call last):
      File "/Users/buildbot-sage/slave/sage_git/build/local/lib/python3.8/site-packages/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/buildbot-sage/slave/sage_git/build/local/lib/python3.8/site-packages/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.package.is_package_installed[3]>", line 2, in <module>
        assert not is_package_installed(pkg), "pip package is installed: {}".format(pkg)
    AssertionError: pip package is installed: zope_interface
**********************************************************************
File "src/sage/misc/package.py", line 454, in sage.misc.package.standard_packages
Failed example:
    installed[0], installed[-1]  # optional - build
Expected:
    ('alabaster', 'zn_poly')
Got:
    ('alabaster', 'zope_interface')
**********************************************************************

comment:15 Changed 17 months ago by mkoeppe

  • Dependencies changed from #30747 to #30747, #30900

comment:16 Changed 17 months ago by git

  • Commit changed from 78bae1ff5356529af4246bc3d33162a6045dde37 to b4927e1574e6f856f5dda78b888713f0358b16da

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

945abb9build/pkgs/zope_interface: Remove
c46e7e4src/sage/misc/package.py: Remove zope_interface from doctests
3bf4ff3Merge branch 'u/isuruf/environment_yaml' of git://trac.sagemath.org/sage into t/30900/remove_last_traces_of_zope_interface
24d7971build/pkgs/zope_interface/distros/conda.txt: Remove
a50ddf8Merge branch 't/28745/environment_yaml' into t/30900/remove_last_traces_of_zope_interface
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

comment:17 Changed 17 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:18 Changed 17 months ago by git

  • Commit changed from b4927e1574e6f856f5dda78b888713f0358b16da to 78ff9d580f5f0c1396e1f8a5421858170999957a

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

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

comment:19 Changed 17 months ago by mkoeppe

  • Cc thansen jhpalmieri added

comment:20 Changed 17 months ago by mkoeppe

  • Cc chapoton added

comment:21 Changed 17 months ago by mkoeppe

Let's please get this ticket in.

comment:22 Changed 17 months ago by mkoeppe

  • Cc gh-kliem added

comment:23 Changed 17 months ago by gh-tobiasdiez

  • Reviewers changed from Tobias Diez to Tobias Diez, ...

comment:24 Changed 17 months ago by mkoeppe

  • Cc slelievre added

comment:25 Changed 17 months ago by gh-kliem

I would expect an empty line before and after

        def normalize(name):
            if normalization is None:
                return name
            elif normalization == 'spkg':
                return name.lower().replace('-', '_').replace('.', '_')
            else:
                raise NotImplementedError(f'normalization {normalization} is not implemented')
Last edited 17 months ago by slelievre (previous) (diff)

comment:26 follow-up: Changed 17 months ago by gh-kliem

Why do we do it as a string here?

pkg['remote_version'] = 'none'

comment:27 in reply to: ↑ 26 Changed 17 months ago by mkoeppe

Replying to gh-kliem:

Why do we do it as a string here?

pkg['remote_version'] = 'none'

This matches what m4/sage_spkg_collect.m4 reports as the version of a package without package-version.txt

comment:28 Changed 17 months ago by git

  • Commit changed from 78ff9d580f5f0c1396e1f8a5421858170999957a to e9a7572a37b228f4a74c35f680190e6b237f48d8

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

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

comment:29 Changed 17 months ago by mkoeppe

  • Dependencies #30747, #30900 deleted

comment:30 Changed 17 months ago by mkoeppe

  • Priority changed from minor to critical

comment:31 Changed 17 months ago by gh-kliem

  • Reviewers changed from Tobias Diez, ... to Tobias Diez, Jonathan Kliem
  • Status changed from needs_review to positive_review

LGTM.

comment:32 Changed 17 months ago by mkoeppe

Thanks!

comment:33 Changed 17 months ago by vbraun

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