Opened 18 months ago
Closed 17 months ago
#30940 closed enhancement (fixed)
src/bin/sagelistpackages: Make it work if SAGE_ROOT is not available
Reported by:  mkoeppe  Owned by:  

Priority:  critical  Milestone:  sage9.3 
Component:  build  Keywords:  sd111 
Cc:  vdelecroix, ghtobiasdiez, slabbe, thansen, jhpalmieri, chapoton, ghkliem, 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: 
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
 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
 Commit set to 3cc5edf85cb2c074ad12029637e09d799d7bde6f
comment:3 Changed 18 months ago by
 Cc vdelecroix added
comment:4 Changed 18 months ago by
 Status changed from new to needs_review
comment:5 Changed 18 months ago by
 Cc ghtobiasdiez added
comment:6 followups: ↓ 7 ↓ 9 Changed 18 months ago by
 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
Replying to ghtobiasdiez:
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
 Commit changed from 3cc5edf85cb2c074ad12029637e09d799d7bde6f to 7f80e3e91f176d1650dc2f3bbb7a25eef366e068
Branch pushed to git repo; I updated commit sha1. New commits:
97942e3  sage.misc.package.pip_installed_packages: Only normalize when optional arg normalization='spkg'

6706182  sage.misc.package.list_packages: Return early if SAGE_PKGS is not available

9258849  sage.misc.package.list_packages: Streamline handling of exclude_pip

7f80e3e  sage.misc.package.list_packages: Actually add new items to the list

comment:9 in reply to: ↑ 6 Changed 18 months ago by
 Cc slabbe added
 Status changed from needs_work to needs_review
Replying to ghtobiasdiez:
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
 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
 Status changed from needs_review to positive_review
Codewise it looks good to me now, thanks for the update!
comment:12 Changed 18 months ago by
 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:
78bae1f  Merge 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
 Status changed from needs_review to positive_review
Merged current beta
comment:14 Changed 17 months ago by
 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/buildbotsage/slave/sage_git/build/local/lib/python3.8/sitepackages/sage/doctest/forker.py", line 714, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/buildbotsage/slave/sage_git/build/local/lib/python3.8/sitepackages/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
 Dependencies changed from #30747 to #30747, #30900
comment:16 Changed 17 months ago by
 Commit changed from 78bae1ff5356529af4246bc3d33162a6045dde37 to b4927e1574e6f856f5dda78b888713f0358b16da
Branch pushed to git repo; I updated commit sha1. New commits:
945abb9  build/pkgs/zope_interface: Remove

c46e7e4  src/sage/misc/package.py: Remove zope_interface from doctests

3bf4ff3  Merge branch 'u/isuruf/environment_yaml' of git://trac.sagemath.org/sage into t/30900/remove_last_traces_of_zope_interface

24d7971  build/pkgs/zope_interface/distros/conda.txt: Remove

a50ddf8  Merge branch 't/28745/environment_yaml' into t/30900/remove_last_traces_of_zope_interface

3cf5ee4  Merge 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

b4927e1  sage.misc.package: Remove/adjust nonrobust doctests

comment:17 Changed 17 months ago by
 Status changed from needs_work to needs_review
comment:18 Changed 17 months ago by
 Commit changed from b4927e1574e6f856f5dda78b888713f0358b16da to 78ff9d580f5f0c1396e1f8a5421858170999957a
comment:19 Changed 17 months ago by
 Cc thansen jhpalmieri added
comment:20 Changed 17 months ago by
 Cc chapoton added
comment:21 Changed 17 months ago by
Let's please get this ticket in.
comment:22 Changed 17 months ago by
 Cc ghkliem added
comment:23 Changed 17 months ago by
 Reviewers changed from Tobias Diez to Tobias Diez, ...
comment:24 Changed 17 months ago by
 Cc slelievre added
comment:25 Changed 17 months ago by
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')
comment:26 followup: ↓ 27 Changed 17 months ago by
Why do we do it as a string here?
pkg['remote_version'] = 'none'
comment:27 in reply to: ↑ 26 Changed 17 months ago by
Replying to ghkliem:
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 packageversion.txt
comment:28 Changed 17 months ago by
 Commit changed from 78ff9d580f5f0c1396e1f8a5421858170999957a to e9a7572a37b228f4a74c35f680190e6b237f48d8
comment:29 Changed 17 months ago by
 Dependencies #30747, #30900 deleted
comment:30 Changed 17 months ago by
 Priority changed from minor to critical
comment:31 Changed 17 months ago by
 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
Thanks!
comment:33 Changed 17 months ago by
 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
New commits:
sage.misc.package: Deprecate standard_packages, optional_packages, experimental_packages
sage.misc.package.pip_installed_packages: Normalize package names
sage.misc.package.list_packages: Merge package lists from installation records and build/pkgs
sage.misc.package.installed_packages: Omit .dummy and other dot files
sage.misc.package.is_package_installed: Remove outdated code