Opened 3 years ago

Closed 2 years ago

#28918 closed enhancement (invalid)

Make giacpy_sage a standard package

Reported by: Samuel Lelièvre Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: packages: optional Keywords:
Cc: Han Frederic, Markus Wageringel, Samuel Lelièvre, Vincent Delecroix Merged in:
Authors: Reviewers: Markus Wageringel
Report Upstream: N/A Work issues:
Branch: public/giacpy_sage-0.7 (Commits, GitHub, GitLab) Commit: 7a4bdb93fbf73c586b4e0cb8ee5235334b284d8b
Dependencies: Stopgaps:

Status badges

Description (last modified by Samuel Lelièvre)

This is to make giacpy_sage a standard SPKG.

It provides Cython bindings to Giac and has been an optional SPKG for several years, initially as giacpy, then since July 2016 as giacpy_sage.

Making it standard would be a step toward replacing all pexpect calls to Giac by giacpy_sage calls, as suggested in

Home:

Upstream upgrade needed (2 doctests failure with python3):

Note: this ticket was made obsolete by

  • #29171 "Move giacpy_sage into sage source code" (merged in Sage 9.2.beta10)

Change History (22)

comment:1 Changed 3 years ago by Samuel Lelièvre

Description: modified (diff)
Milestone: sage-9.0sage-9.1

comment:2 Changed 3 years ago by Markus Wageringel

What is required to make this happen?

comment:3 in reply to:  2 Changed 3 years ago by François Bissey

Replying to gh-mwageringel:

What is required to make this happen?

Usually a vote on sage-devel. The package has been optional long enough that it should be the only requirement.

For info, the only case new packages go straight to standard without vote or questions is when they are new dependency of a standard package.

comment:4 Changed 3 years ago by Samuel Lelièvre

comment:5 Changed 3 years ago by Han Frederic

Thank you for this ticket. NB: if the vote is favorable we need to remove all the:

optional - giacpy_sage

in the comments of the doctests in the following files:

src/sage/libs/giac.py
src/sage/rings/polynomial/multi_polynomial_ideal.py

NB: the random and long time comments should stay.

There is also a flag in the following

build/pkgs/giacpy_sage/type

but I don't know if the ticket should change it or let the Release Manager do it himself.

comment:6 Changed 3 years ago by Han Frederic

Authors: Frederic Han
Branch: public/giacpy_sage-0.7
Commit: 85e78011637ab7fc553e3d1dd726311676412ec4
Description: modified (diff)

I have found 2 doctest failure in sage 9.1.beta3 due to python3. So we also need to upgrade the upstream source to giacpy_sage-0.7.0.

I have started a new public branch with this upstream fix. But let all the optional flags in the sage code. If the vote turn out to be positive we will have to remove them.

comment:7 Changed 3 years ago by François Bissey

Something for upstream, I fail to compile giacpy_sage-0.7.0 with the latest giac (1.5.0.85)

building 'giacpy_sage' extension
creating /dev/shm/portage/dev-python/giacpy_sage-0.7.0/work/giacpy_sage-0.7.0-python3_7/temp.linux-x86_64-3.7
x86_64-pc-linux-gnu-g++ -march=native -O2 -pipe -fPIC -I/usr/lib/python3.7/site-packages/cysignals -I/dev/shm/portage/dev-python/giacpy_sage-0.7.0/work/giacpy_sage-0.7.0 -I/usr/lib/python3.7/site-packages/sage/libs/ntl -I/usr/lib/python3.7/site-packages/sage/cpython -I/usr/include -I/usr/lib/python3.7/site-packages -I/usr/lib/python3.7/site-packages/sage/ext -I/usr/include/python3.7m -I/usr/lib/python3.7/site-packages/numpy/core/include -I/usr/include/python3.7m -c giacpy_sage.cpp -o /dev/shm/portage/dev-python/giacpy_sage-0.7.0/work/giacpy_sage-0.7.0-python3_7/temp.linux-x86_64-3.7/giacpy_sage.o -std=c++11
In file included from giacpy_sage.cpp:665:
misc.h: In function ‘void archivegen(std::string, const giac::gen&, const giac::context*)’:
misc.h:104:15: error: variable ‘std::ofstream of’ has initializer but incomplete type
  104 |   ofstream of(filename.c_str());
      |               ^~~~~~~~
misc.h: In function ‘giac::gen unarchivegen(std::string, const giac::context*)’:
misc.h:110:14: error: variable ‘std::ifstream f’ has initializer but incomplete type
  110 |   ifstream f(filename.c_str());
      |              ^~~~~~~~
/usr/lib/python3.7/site-packages/Cython/Compiler/Main.py:369: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: /dev/shm/portage/dev-python/giacpy_sage-0.7.0/work/giacpy_sage-0.7.0/giacpy_sage.pxd
  tree = Parsing.p_module(s, pxd, full_module_name)
error: command 'x86_64-pc-linux-gnu-g++' failed with exit status 1

Any ideas?

Last edited 3 years ago by François Bissey (previous) (diff)

comment:8 Changed 3 years ago by François Bissey

I actually have the same issue with giacpy_sage 0.6.8.

comment:9 Changed 3 years ago by Han Frederic

Thank you François, it's a missing include. I will fix it in giacpy. It seems that between giac-1.5.0.63 and 1.5.0.75 fstream was removed in several places:

fred@localhost src]$ grep "<fstream>" *.h
global.h://#include <fstream>
Graph3d.h:#include <fstream>
graphe.h:#include <fstream>
Graph.h:#include <fstream>
help.h://#include <fstream>
monomial.h://#include <fstream>
opengl.h:#include <fstream>
poly.h://#include <fstream>

[fred@localhost src]$ grep "<fstream>" ~/dev/sage/develop/sage/local/include/giac/*.h
/home/fred/dev/sage/develop/sage/local/include/giac/global.h:#include <fstream>
/home/fred/dev/sage/develop/sage/local/include/giac/graphe.h:#include <fstream>
/home/fred/dev/sage/develop/sage/local/include/giac/help.h:#include <fstream>
/home/fred/dev/sage/develop/sage/local/include/giac/monomial.h:#include <fstream>
/home/fred/dev/sage/develop/sage/local/include/giac/poly.h:#include <fstream>

comment:10 Changed 3 years ago by git

Commit: 85e78011637ab7fc553e3d1dd726311676412ec47a4bdb93fbf73c586b4e0cb8ee5235334b284d8b

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

7a4bdb9update to giacpy_sage-0.7.1 to fix a missing include with giac >1.5.0.75

comment:11 Changed 3 years ago by Han Frederic

Description: modified (diff)

the missing header with giac >1.5.0.75 reported by francois, should be fixed in giacpy_sage-0.7.1 I have update the branch accordingly.

comment:12 in reply to:  11 Changed 3 years ago by François Bissey

Replying to frederichan:

the missing header with giac >1.5.0.75 reported by francois, should be fixed in giacpy_sage-0.7.1 I have update the branch accordingly.

Works now. Thanks!

comment:13 Changed 3 years ago by Matthias Köppe

Milestone: sage-9.1sage-9.2

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

comment:14 Changed 2 years ago by Matthias Köppe

See also #29552

comment:15 Changed 2 years ago by Matthias Köppe

Cc: Vincent Delecroix added
Priority: majorcritical

comment:16 Changed 2 years ago by Matthias Köppe

Status: newneeds_info

comment:17 Changed 2 years ago by François Bissey

I have issues with this going to standard by just flipping the switch. It risks to become a circular dependency madness.

Currently giacpy_sage requires sage at build time to figure out the include directory by importing sage_include_directory from sage.env. That part is not needed and should probably be simplified. If you are installing via sage -i the environment should be set correctly for finding the headers. If you are on a distro they also are in reach. Similarly library_dirs doesn't need to be set (in fact the current setup is annoying on systems with lib/lib64 split). But sage is needed when compiling the .pyx files as it imports a fair deal of sage bits.

sage is needed at runtime and test time.

If sage needs the package at runtime or to build the documentation you create a scenario where building sage look like:

  1. build sagelib
  2. build giacpy_sage
  3. optional: test giacpy_sage
  4. build sagelib documentation
  5. test sagelib

So giacpy_sage is inserting itself in the middle of the traditional build cycle.

sage_brial look superficially similar but in its case it can be installed independently of sage so it can go as step 1 (it cannot be tested however). sage_brial should in fact be merged into sagelib and I believe merging giacpy_sage inside sagelib is the only way to get out what looks like circular dependency in the future.

comment:18 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-duplicate/invalid/wontfix
Status: needs_infoneeds_review

Yes, I agree that #29171, merging into sagelib, is the way to go.

comment:19 in reply to:  17 Changed 2 years ago by Matthias Köppe

Replying to fbissey:

sage_brial look superficially similar but in its case it can be installed independently of sage so it can go as step 1 (it cannot be tested however). sage_brial should in fact be merged into sagelib

This is now #30332

comment:20 Changed 2 years ago by Markus Wageringel

Authors: Frederic Han
Reviewers: Markus Wageringel
Status: needs_reviewpositive_review

comment:21 Changed 2 years ago by Matthias Köppe

Priority: criticalmajor

comment:22 Changed 2 years ago by Samuel Lelièvre

Description: modified (diff)
Resolution: invalid
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.