downgrade giac to optional

[dimpase]

This will resolve #38668

It's actually almost done, we just need to change the package type, and make sure that an external
giac does not get picked up by feature if --disable-giac is in effect.

For the latter, we substitute the value of an automatically generated variable
SAGE_ENABLE_giac into a variable in sage.env, and use it in sage.feature.giac
to poison the executable name if giac is disabled.
(maybe there is a better than poisoning way)

Testing:

./bootstrap
make giac-clean giac-uninstall
./configure --disable-giac
# ... run tests ...
./configure --enable-giac
# ... run tests ...

📝 Checklist

• [ x] The title is concise and informative.
• [ x] The description explains in detail what this PR is about.
• [x ] I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

⌛ Dependencies

• src/sage/features/giac.py: add new feature for the giac program #38672 src/sage/features/giac.py: add new feature for the giac program
• Tweak a few tests to pass when giac is not installed #38690 Tweak a few tests to pass when giac is not installed

[dimpase]

The dependencies are rebased over the latest beta5

[github-actions]

Documentation preview for this PR (built with commit a451352; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[dimpase]

This works if giac needs to be built. But if a system-wide giac is detected and approved for use, then feature giac is detected, and doctests are run, even though we don't want it in case we configure with --disable-giac

So --disable-giac should include stopping feature giac from being detected. Currently it's not the case.

EDIT - fixed by d47e633

[dimpase]

I tried testing locally, this "Build & Test / test-new (pull_request)" seems to be failing due to a shortcut taken by this CI step.
All works for me locally.

[antonio-rojas]

Is this supposed to make it optional only at runtime? Because it still fails to build if giac is not present.

[dimpase]

to build, you need to configure with --enable-giac (as it's the case for any optional package)
Have you tried this?

[antonio-rojas]

I'm building sagelib (aka sagemath-standard), not sage-the-distro. And if giac is not installed, build fails with

build/cythonized/sage/libs/giac/giac.cpp:1286:10: fatal error: giac/giac.h: No such file or directory

[dimpase]

I've added explicit testing instructions in the description

[mkoeppe]

Of course the extension modules can no longer be built and shipped by sagemath-standard when the package is made optional. They need to go into a new distribution, along the lines of sagemath-bliss etc. (and sagemath-brial as done in #36380.

[dimpase]

I'm building sagelib (aka sagemath-standard), not sage-the-distro. And if giac is not installed, build fails with

build/cythonized/sage/libs/giac/giac.cpp:1286:10: fatal error: giac/giac.h: No such file or directory

Why would you expect sagelib to be able to build giac? Building giac is something entirely different.

As we don't have a standard, documented, procedure for building a standalone sagelib,
I really don't see what you're doing. If your undocumented way to build sagelib is not working, it doesn't mean that this ticket is not doing what's advertised to do - downgrade giac to optional within the only documented way to build Sage.

We really should get on with getting a common way to build sagelib standalone, in particular as it's basically done in #36524, so we should get on with getting #36524 fixed and merged, document it, etc. @tobiasdiez

[kiwifb]

@dimpase me and a lot of other distro have managed to build sage as modular parts and in fact it is the only proper way to do it anymore as a distro in my opinion.

sage upstream (aka you) should get on and eat your own food properly. One thing modular sage has done for us that is extremely positive is split those optional packages that had a terrible build system involving detection in sagelib at build time. Now we can build them independently and that's a good thing(TM).

And if you are going to make giac optional, I am expecting you to go all the way and create a sagemath-giac package. Anything else is half backed and will be disputed by me.

[mkoeppe]

We really should get on with getting a common way to build sagelib standalone

@dimpase What are you talking about. The sagemath-standard distribution on PyPI exists for this very purpose.

[mkoeppe]

@kiwifb Note that the sagemath-giac package is defined already in #35095, and I can split it out from there after #36380 is merged.

[dimpase]

We really should get on with getting a common way to build sagelib standalone

What are you talking about. The sagemath-standard distribution on PyPI exists for this very purpose.

There is no documentation on how sagemath-standard is built. There is some talk about sagemath-standard in src/doc/en/developer/packaging_sage_library.rst, but it leaves one in the dark on how one can build it in a stand-alone mode.

One can put as much sagemath-* stuff on PyPI as one pleases, but it's not replacing documentation.

[dimpase]

@dimpase me and a lot of other distro have managed to build sage as modular parts and in fact it is the only proper way to do it anymore as a distro in my opinion.

I fully agree with you here.

sage upstream (aka you) should get on and eat your own food properly.

We have a problem defining the diet, as you know.
You somehow on one hand want "to build sage as modular parts" (and I want this too), and on the other hand don't want
to facilitate this here, instead pulling Sage towards even tighter integration of sagelib into sage the distro.

One thing modular sage has done for us that is extremely positive is split those optional packages that had a terrible build system involving detection in sagelib at build time. Now we can build them independently and that's a good thing(TM).

And if you are going to make giac optional, I am expecting you to go all the way and create a sagemath-giac package. Anything else is half backed and will be disputed by me.

giac is much more half-baked, as you perfectly know. I don't want to spend my time writing endless undocumented configs for something that's broken beyond repair.

If you want a sagemath-giac package so badly, you can work on it in a follow-up PR, but downgrading a package to optional should not take more time and effort than is already taken. It used to be easy, and it should stay so.

[dimpase]

@kiwifb Note that the sagemath-giac package is defined already in #35095, and I can split it out from there after #36380 is merged.

Why is splitting sagemath-giac package from the 4K-commits long patchbomb #35095 touching almost 4K files requires merging a seemingly unrelated #36380 ?

[dimpase]

"Undocumented" means that documentation is not written.
There are other words for not reading the documentation.

Please point out at the place where creating packages such as sagemath-brial is documented in sufficient detail (or even is some detail). There is no such place. Or point me to this place, because I cannot find it.

[tobiasdiez]

@dimpase You probably want to manually add the files that need giac (like giac.pyx) to

sage/src/setup.py

Line 83 in d1f99d1

files_to_exclude = filter_cython_sources(SAGE_SRC, distributions_to_exclude)

when it's not available. Otherwise pip install will try to compile them, which fails with the error mentioned in #38712 (comment). This will indeed become way easier and, in fact, automatic with the Meson build system which is able to track such dependencies.

[dimpase]

@kiwifb Note that the sagemath-giac package is defined already in #35095, and I can split it out from there after #36380 is merged.

Why can't you split it out before unrelated to it #36380 is merged?

If you do it, add it here (the PRs on https://github.com/dimpase/sage are open), we'll get this PR done and dusted, instead of starting yet another fight. Or do it in a follow-up PR, if you prefer, do not block this one.

[dimpase]

@dimpase You probably want to manually add the files that need giac (like giac.pyx) to

sage/src/setup.py

Line 83 in d1f99d1

files_to_exclude = filter_cython_sources(SAGE_SRC, distributions_to_exclude)

when it's not available. Otherwise pip install will try to compile them, which fails with the error mentioned in #38712 (comment). This will indeed become way easier and, in fact, automatic with the Meson build system which is able to track such dependencies.

Thanks for the pointer! I see in that file

optional_packages = ['mcqd', 'bliss', 'tdlib',
                         'coxeter3', 'sirocco', 'meataxe']

which appears to be a semi-random list of optional packages - or are these "distributions" ?

Yes, indeed, these are used to form sagemath- names, of things in pkgs/:

$ ls pkgs/
README.rst       sage-conf_pypi  sagemath-brial       sagemath-environment  sagemath-objects  sagemath-standard  sage-sws2rst
sage-conf        sage-docbuild   sagemath-categories  sagemath-mcqd         sagemath-repl     sagemath-tdlib
sage-conf_conda  sagemath-bliss  sagemath-coxeter3    sagemath-meataxe      sagemath-sirocco  sage-setup

---

By the way, why isn't pkgs/ split into pkgs/standard/ and pkgs/optional/ ? For more confusion, and maintaining lists by hand?

[mkoeppe]

Why can't you split it out before unrelated to it #36380 is merged?

It's not "unrelated". The giac bindings likely also need the enlarged sagemath-categories done in #36380.

[mkoeppe]

Why isn't pkgs/ split into pkgs/standard/ and pkgs/optional/ ?

Because that would be less convenient.

[mkoeppe]

list of optional packages - or are these "distributions" ?

"distribution" is the same as "distribution package" is the same as "pip-installable package".

[tobiasdiez]

@dimpase You probably want to manually add the files that need giac (like giac.pyx) to

sage/src/setup.py

Line 83 in d1f99d1

files_to_exclude = filter_cython_sources(SAGE_SRC, distributions_to_exclude)

when it's not available. Otherwise pip install will try to compile them, which fails with the error mentioned in #38712 (comment). This will indeed become way easier and, in fact, automatic with the Meson build system which is able to track such dependencies.

Thanks for the pointer! I see in that file

optional_packages = ['mcqd', 'bliss', 'tdlib',
                         'coxeter3', 'sirocco', 'meataxe']

which appears to be a semi-random list of optional packages - or are these "distributions" ?

Yes, indeed, these are used to form sagemath- names, of things in pkgs/:

They are indeed intended to be used in conjunction with sagemath-xyz distributions. But in this context, they essentially match whatever files have the magic distribution-comment at the beginning. So if you want to exclude certain "giac" files, just add giac to the optional_packages array and add # sage_setup: distribution = sagemath-giac at the beginning of the files in src/sage/libs/giac. No need to create a whole distribution package (which apparently is hard problem).

[dimpase]

Why isn't pkgs/ split into pkgs/standard/ and pkgs/optional/ ?

Because that would be less convenient.

I don't see how maintaining explicit lists of optional things, while mixing up non-optional and optional things in the same directory is more convenient than putting optional things in optional/
Using optional/ is certainly less work and easier to maintain, once the framework to use such things is available. Because it means less places to touch, no need to worry about a list somewhere else.

[orlitzky]

It may be easier to do this in a few steps to ensure that no one objects to them. A few related PRs were merged into develop recently, and #38686 should go in next time. After that, I think the uncontroversial thing to do would be to add only the # needs giac tags. That changes nothing so long as giac is still standard so is less likely to draw objections. Afterwards it will be easier to focus on whatever we need to do w.r.t. sagemath-giac.

[dimpase]

@orlitzky - I am not sure what you are proposing to be split here. I think the modifications in this branch are needed regardless.

[orlitzky]

@orlitzky - I am not sure what you are proposing to be split here. I think the modifications in this branch are needed regardless.

Not split per se, but I think you are getting a lot of negative comments because this branch would break some things without the prerequisite # needs tags and some way to separate the building of sage.libs.giac. If we are able to get those prerequisites merged first then the subsequent discussion will be much more focused.

[dimpase]

I think I've already put in the equivalents of #needs tags in. There is a per file analog of these (# sage.doctest: optional - giac), so that one doesn't need per doctest tags in src/sage/interfaces/giac*.

Perhaps this tag is not needed in src/sage/features/giac*, not sure.

[orlitzky]

They are indeed intended to be used in conjunction with sagemath-xyz distributions. But in this context, they essentially match whatever files have the magic distribution-comment at the beginning. So if you want to exclude certain "giac" files, just add giac to the optional_packages array and add # sage_setup: distribution = sagemath-giac at the beginning of the files in src/sage/libs/giac. No need to create a whole distribution package (which apparently is hard problem).

This removes sage.libs.giac from the default build, but without the corresponding sagemath-giac distribution, I think there's no easy way to re-enable it.

The giac bindings likely also need the enlarged sagemath-categories done in #36380.

@mkoeppe would you be willing to split it out to see what happens? After #38770 this is the last step.

[tobiasdiez]

They are indeed intended to be used in conjunction with sagemath-xyz distributions. But in this context, they essentially match whatever files have the magic distribution-comment at the beginning. So if you want to exclude certain "giac" files, just add giac to the optional_packages array and add # sage_setup: distribution = sagemath-giac at the beginning of the files in src/sage/libs/giac. No need to create a whole distribution package (which apparently is hard problem).

This removes sage.libs.giac from the default build, but without the corresponding sagemath-giac distribution, I think there's no easy way to re-enable it.

I thought about adding giac to optional_packages only if sage.env.SAGE_GIAC_ENABLED is false. So you would be able to control building the giac-part via the environment variable. Alternatively, one could use is_package_installed_and_updated to check for giac at build time.

Or create a giac distribution but make it require sagelib during build time, then one doesn't need to change sage-categories for it to build successful.

[kiwifb]

Alternatively, one could use is_package_installed_and_updated to check for giac at build time.

Please no. Just no. My opinion on this is well known, we should not mix sage meta build package management with "sagelib" build or runtime system.

[antonio-rojas]

Alternatively, one could use is_package_installed_and_updated to check for giac at build time.

Please no. Just no. My opinion on this is well known, we should not mix sage meta build package management with "sagelib" build or runtime system.

+1. is_package_installed should never be used in sagelib.

[tobiasdiez]

Alternatively, one could use is_package_installed_and_updated to check for giac at build time.

Please no. Just no. My opinion on this is well known, we should not mix sage meta build package management with "sagelib" build or runtime system.

Okay, fair enough. For another PR: should we remove sage.misc.package from sagelib and put it somewhere in sage-the-distro? Looking at the code, it appears this doesn't make much sense without sage-the-distro?

[kiwifb]

Alternatively, one could use is_package_installed_and_updated to check for giac at build time.

Please no. Just no. My opinion on this is well known, we should not mix sage meta build package management with "sagelib" build or runtime system.

Okay, fair enough. For another PR: should we remove sage.misc.package from sagelib and put it somewhere in sage-the-distro? Looking at the code, it appears this doesn't make much sense without sage-the-distro?

I would be in favor of that.