Remove unused class from sage_setup.optional extension

[tobiasdiez]

The code in optional_extension was mostly unused and is removed in this ticket (and similarly extension.skip_build was nowhere else set).
The only still used method was is_package_installed_and_updated which is moved to sage.misc.package.

Related: #28815

CC: @mkoeppe @kiwifb

Component: build

Keywords: sd111

Author: Tobias Diez

Branch/Commit: a7b1e81

Reviewer: François Bissey

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

[sagetrac-git]

Changed commit from 508075c to 2492f1e

[sagetrac-git]

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

2492f1e

Code cleanup

[mkoeppe]

comment:4

Can't make incompatible changes to the interface of a public function

[mkoeppe]

comment:5

(I'm referring to the list_packages function)

[mkoeppe]

comment:6

It's also conflicting with the changes in #30940 (which needs review)

[sagetrac-git]

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

649ffff

Fix build

[sagetrac-git]

Changed commit from 2492f1e to 649ffff

[tobiasdiez]

comment:8

Replying to @mkoeppe:

Can't make incompatible changes to the interface of a public function

So one cannot change public facing functions at all? What is the policy/strategy for this? Introduce a new method, say list_packages_typed?

It's also conflicting with the changes in #30940 (which needs review)

I wouldn't say it's conflicting, just results in easy-to-resolve merge conflicts.

[mkoeppe]

comment:9

Replying to @tobiasdiez:

Replying to @mkoeppe:

Can't make incompatible changes to the interface of a public function

So one cannot change public facing functions at all? What is the policy/strategy for this? Introduce a new method, say list_packages_typed?

It could be a new function (with deprecation for the old one), or an optional argument that controls the return type, ...

But in this particular case, it would probably be good enough to just give the new class dictionary-like behavior so that user code can continue to access the slots like a dictionary. Possibly with deprecation warning.

[sagetrac-git]

Changed commit from 649ffff to 26b59ad

[sagetrac-git]

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

26b59ad

Implement __getitem__

[tobiasdiez]

comment:11

Replying to @mkoeppe:

But in this particular case, it would probably be good enough to just give the new class dictionary-like behavior so that user code can continue to access the slots like a dictionary. Possibly with deprecation warning.

That's a nice idea, implemented!

[mkoeppe]

comment:12

Instead of "string-based access", perhaps this should be called "dict-like access" in the deprecation messages?

[mkoeppe]

comment:13

I think in this change you need to use is_installed() instead

--- a/src/bin/sage-list-packages
+++ b/src/bin/sage-list-packages
@@ -91,16 +91,16 @@ else:
 if WARN:
     print(WARN)
 if args['installed_only']:
-    L = [pkg for pkg in L if pkg['installed']]
+    L = [pkg for pkg in L if pkg.installed]
 elif args['not_installed_only']:
-    L = [pkg for pkg in L if not pkg['installed']]
+    L = [pkg for pkg in L if not pkg.installed]

[mkoeppe]

comment:16

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

[mkoeppe]

Changed keywords from none to sd111

[sagetrac-git]

Changed commit from 26b59ad to 0c10be0

[sagetrac-git]

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

0c10be0

Fix build

[tobiasdiez]

comment:18

Replying to @mkoeppe:

I think in this change you need to use is_installed() instead

Indeed, thanks!

[mkoeppe]

comment:61

Setting a new milestone for this ticket based on a cursory review.

[sagetrac-git]

Changed commit from 541fa1b to 6c67908

[sagetrac-git]

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

6c67908

Merge branch 'develop' of https://github.com/sagemath/sage into public/build/remove_optional_extension

[tobiasdiez]

comment:63

Rebased, so this is now ready for review.

[tobiasdiez]

Changed work issues from rebase on #31385 to none

[mkoeppe]

comment:64

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

[tobiasdiez]

comment:65

Yes, I would appreciate if this ticket could be reviewed!

[sagetrac-git]

Changed commit from 6c67908 to a7b1e81

[sagetrac-git]

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

11b45bd	Merge branch 'develop' of https://github.com/sagemath/sage into public/build/remove_optional_extension
a7b1e81	Improve typing

[tobiasdiez]

Changed dependencies from #31385 to none

[kiwifb]

comment:69

Thanks for copying me Matthias. I was aware of this ticket and I guess it will at least remove some pesky doctests. Otherwise it will just make me move some patch from one file to another.

is_package_installed_and_updated is one of those things I am patiently waiting for the modularisation of sage to kill. In the meantime I am patching an alternative mechanism anyway.

[kiwifb]

comment:70

Other than that, if you copied me for the review, it all looks good and sensible to me.

[kiwifb]

Reviewer: François Bissey

[tobiasdiez]

comment:71

Thanks for the review!

[vbraun]

Changed branch from public/build/remove_optional_extension to a7b1e81