Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for --disable-giac #38668

Open
orlitzky opened this issue Sep 17, 2024 · 10 comments · May be fixed by #38712
Open

Support for --disable-giac #38668

orlitzky opened this issue Sep 17, 2024 · 10 comments · May be fixed by #38712

Comments

@orlitzky
Copy link
Contributor

orlitzky commented Sep 17, 2024

Giac is a PITA to have as a dependency:

  • There's no public source repository
  • There's no bug tracker
  • There are no release announcements, new releases just show up in some random directory...
  • If you can tell what is the latest release, because there's no coherent version scheme
  • No one knows what's in the new releases, breaking changes show up in what appear to be minor versions
  • The tarball is manually generated and often contains random files from the author's computer
  • The test suite for every version fails on almost every machine
  • Recent releases contain undefined behavior that is retained intentionally even though it crashes glibcxx
  • The build emits hundreds of compiler warnings
  • As a result of all of this, each release requires heavy patching (i.e. time & effort) even on "boring" systems
  • But now I have a new computer that is not boring, and those compiler warnings weren't kidding, because giac segfaults all over the place
  • So I can't use giac any more, and if Sage has a hard dependency on it, then I can't use Sage any more either

Fortunately it looks like it won't be too hard to make giac optional in Sage:

  1. Remove giac integration from GiNaC #38669
  2. src/sage/features/giac.py: add new feature for the giac program #38672
  3. Tweak a few tests to pass when giac is not installed #38690
  4. src/sage/symbolic/integration: make libgiac integration optional #38756
  5. Use the faster libgiac interface for "giac" integration #38686 (not strictly required, but it conflicts with several "needs" tags)
  6. Add "needs" tags for giac and libgiac #38770
  7. Add the --disable-giac flag to ./configure
  8. Figure out how to disable the building of sage.libs.giac when libgiac is not found

We lose a few integration examples in the doctests when the giac algorithm is skipped in favor of sympy, but that's about it.

@parisseb
Copy link

I wonder if it's really worth replying such an attack.
For the records, there is a repository for giac source files inside geogebra git repo, changes are commented there. I do not maintain myself a repository. I copy the auto-generated tarballs when I build a binary deb package and it sets the version number. When I build a binary deb, I of course check my own regression tests (I have to make some times adjustements). I'm using glibcxx,without problems once some recent "features" are disabled (this is of course done by the configure script from giac if you don't patch it), because I do not want to modify working source code.
If you want to report a bug, you can do it on Xcas forum.

If sagemath chooses to make giac optional, it might be a problem for some sage users, not for me. There are plenty of users of giac worldwide, either from ports I maintain myself or embedded in other applications.
Bye!

vbraun pushed a commit to vbraun/sage that referenced this issue Sep 19, 2024
Phase one of sagemath#38668

AFAICS it's not doing anything anyway.

URL: sagemath#38669
Reported by: Michael Orlitzky
Reviewer(s):
@kiwifb
Copy link
Member

kiwifb commented Sep 19, 2024

Hi Parisse,
I am going to guess that you refer to this repo https://github.com/geogebra/giac which is quite different from the source tarballs we can find on your site. It has a different build system and I am not sure what we build with it. If this is your official repo, it would be nice to know how it relates to your releases.
Some of your latest source tarballs have come with arm objects and executables. Which kindly interferes with the build process because make does not know about the architecture of the object.
The compile does generate a lot (understatement) of warnings. C++ standard are moving much faster than you do. But if you accept PRs we may be able to catch up on that.
Disabling features and working source code. The end of that logic is usually not to update anything as much as possible. Which means developing in a specific, end of life, version of Linux in a Virtual Machine and exclusively distributing the application as containers produced on that machine - maybe I am too modern, virtual machine images would be safer. It means your application has now a lot in common with appliance such as dishwashers and washing machines (without IoT) - not necessarily a bad thing so long as we are clear about it.

@parisseb
Copy link

Geogebra has indeed it's own build system, but the source files and header files of giac are the same as in the giac tarball (except for the FLTK Xcas UI). The giac tarball is a unique source for the giac library and Xcas UI, that's the reason why it contains files relative to Xcas, like ARM32 firmwares port of Xcas for the Numworks calculators (Xcas can handle these calculators).
There are a lot of warnings if you enable all warnings (like unsigned vs signed integers warnings), with the default flags I don't get much warnings (a few while compiling Xcas UI source files).
If you have changes to propose, you can post them on Xcas forum (I believe you already have an account there). The best for me is a tarball with the changed files if there are many changes (I'll do the diff myself and merge within emacs), or just a diff for a small number of changes. I may have to reject some changes (or ifdef them), if they are not compatible with the many ports of giac (including all calculator ports with sometimes old compilers).
Sorry I did not understand your last paragraph.

vbraun pushed a commit to vbraun/sage that referenced this issue Sep 22, 2024
    
Phase one of sagemath#38668

AFAICS it's not doing anything anyway.
    
URL: sagemath#38669
Reported by: Michael Orlitzky
Reviewer(s):
@dimpase
Copy link
Member

dimpase commented Sep 24, 2024

  1. Add # needs giac tags everywhere else

  2. Add the --disable-giac flag to ./configure

  3. Figure out how to disable the building of sage.libs.giac when libgiac is not found

just changing the package type to optional enables --disable-giac.
So I'm going to do this, and then see which doctests fail, and tag them.
Should be done then :-)

@dimpase dimpase linked a pull request Sep 25, 2024 that will close this issue
2 tasks
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 27, 2024
    
This is one of the prerequisites to make it possible to install sage
without giac (sagemath#38668). These few
tests can be made to work easily without a `# needs giac`, so let's do
that.
    
URL: sagemath#38690
Reported by: Michael Orlitzky
Reviewer(s): David Coudert
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 28, 2024
    
This is one of the prerequisites to make it possible to install sage
without giac (sagemath#38668). These few
tests can be made to work easily without a `# needs giac`, so let's do
that.
    
URL: sagemath#38690
Reported by: Michael Orlitzky
Reviewer(s): David Coudert
@orlitzky
Copy link
Contributor Author

For the last step (disabling sage.libs.giac), meson will help in the short term, but for sage-the-distro I think we should move the entire folder into a separate sagemath-giac package. We can have it depend on sagemath-standard rather than -categories and -objects unless someone wants to adopt Matthias's PR.

@dimpase
Copy link
Member

dimpase commented Oct 18, 2024

Frankly, I'd rather see the distributions spaghetti in a freezer or another suitable container. The objections to #38712 seem to stem from the old way(s) of building sagelib, and sagelib only. If you build sage-distro in the usual way, then #38712 pretty much does what's advertised.

With meson build, it should not be a problem, no need to do a deep dive into spaghetti, to produce more of it ?

@orlitzky
Copy link
Contributor Author

Frankly, I'd rather see the distributions spaghetti in a freezer or another suitable container. The objections to #38712 seem to stem from the old way(s) of building sagelib, and sagelib only. If you build sage-distro in the usual way, then #38712 pretty much does what's advertised.

I think if you --disable-giac in #38712 and don't have giac installed, sagelib will fail to build because it still tries to link sage/libs/giac/* against libgiac (which isn't there). I haven't tried your branch recently but I did roughly the same thing and that was the big problem I ran into. Unless you can make sage/libs/giac go away somehow, you'll get build failures.

With meson build, it should not be a problem, no need to do a deep dive into spaghetti, to produce more of it ?

Yes meson will be a big help because it will skip compilation of sage/libs/giac if its dependencies (like libgiac) are not found. At that point there is no real problem for me. Still, I think there is some value to having sagemath-giac as a separate package:

  • It keeps giac and sagelib from getting re-entangled
  • You can upgrade one or the other independently (faster)
  • It will fix the sage/libs/giac problem for non-meson builds in the meantime if it takes us a very long time to finish the meson migration. (In sagelib, we would rm -rf sage/libs/giac and then add sagemath-giac to build/pkgs as optional.)

vbraun pushed a commit to vbraun/sage that referenced this issue Oct 23, 2024
    
Part of sagemath#38668. If it's going to
be possible to disable giac, we need to guard all of the tests that use
it with either `# needs giac` or `# needs sage.libs.giac`.

I think I've gotten them all. A crude way to test:

1. `git rm -r src/sage/libs/giac` and rebuild to disable sage.libs.giac
2. build sage, and then delete the giac executable to disable the
pexpect interface

If you do these one at a time, it should ensure that the correct tags
are used. (Typically, if giac is missing, neither sage.libs.giac nor the
giac executable will be present, making it very easy to mix up the
tags.)

For bonus points you can undelete `src/sage/libs/giac` after building
but before testing to make sure the "needs" tags in those files are
accurate.

### Dependencies:

* sagemath#38756
* sagemath#38686 (not strictly required,
but it adds a few "needs sage.libs.giac" tags of its own)
    
URL: sagemath#38770
Reported by: Michael Orlitzky
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this issue Oct 26, 2024
    
Part of sagemath#38668. If it's going to
be possible to disable giac, we need to guard all of the tests that use
it with either `# needs giac` or `# needs sage.libs.giac`.

I think I've gotten them all. A crude way to test:

1. `git rm -r src/sage/libs/giac` and rebuild to disable sage.libs.giac
2. build sage, and then delete the giac executable to disable the
pexpect interface

If you do these one at a time, it should ensure that the correct tags
are used. (Typically, if giac is missing, neither sage.libs.giac nor the
giac executable will be present, making it very easy to mix up the
tags.)

For bonus points you can undelete `src/sage/libs/giac` after building
but before testing to make sure the "needs" tags in those files are
accurate.

### Dependencies:

* sagemath#38756
* sagemath#38686 (not strictly required,
but it adds a few "needs sage.libs.giac" tags of its own)
    
URL: sagemath#38770
Reported by: Michael Orlitzky
Reviewer(s): Tobias Diez
@vbraun
Copy link
Member

vbraun commented Oct 26, 2024

For the record, giac::zmakelinesplit consistently segfaults the Sage testsuite on the macos M2 buildbot. I could fix it but I don't even know against which repo.

+1 for having a way to disable giac

@parisseb
Copy link

Please post your fix proposal here as well as the command that raised the segfault (probably a gbasis).
Thank you.

@vbraun
Copy link
Member

vbraun commented Oct 27, 2024

I don't want to take over this issue, so I opened #38864 (for lack of an upstream bug tracker)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants