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:  sageduplicate/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_sage0.7 (Commits, GitHub, GitLab)  Commit:  7a4bdb93fbf73c586b4e0cb8ee5235334b284d8b 
Dependencies:  Stopgaps: 
Description (last modified by )
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
Description:  modified (diff) 

Milestone:  sage9.0 → sage9.1 
comment:2 followup: 3 Changed 3 years ago by
comment:3 Changed 3 years ago by
Replying to ghmwageringel:
What is required to make this happen?
Usually a vote on sagedevel. 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:5 Changed 3 years ago by
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
Authors:  → Frederic Han 

Branch:  → public/giacpy_sage0.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_sage0.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
Something for upstream, I fail to compile giacpy_sage0.7.0 with the latest giac (1.5.0.85)
building 'giacpy_sage' extension creating /dev/shm/portage/devpython/giacpy_sage0.7.0/work/giacpy_sage0.7.0python3_7/temp.linuxx86_643.7 x86_64pclinuxgnug++ march=native O2 pipe fPIC I/usr/lib/python3.7/sitepackages/cysignals I/dev/shm/portage/devpython/giacpy_sage0.7.0/work/giacpy_sage0.7.0 I/usr/lib/python3.7/sitepackages/sage/libs/ntl I/usr/lib/python3.7/sitepackages/sage/cpython I/usr/include I/usr/lib/python3.7/sitepackages I/usr/lib/python3.7/sitepackages/sage/ext I/usr/include/python3.7m I/usr/lib/python3.7/sitepackages/numpy/core/include I/usr/include/python3.7m c giacpy_sage.cpp o /dev/shm/portage/devpython/giacpy_sage0.7.0/work/giacpy_sage0.7.0python3_7/temp.linuxx86_643.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/sitepackages/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/devpython/giacpy_sage0.7.0/work/giacpy_sage0.7.0/giacpy_sage.pxd tree = Parsing.p_module(s, pxd, full_module_name) error: command 'x86_64pclinuxgnug++' failed with exit status 1
Any ideas?
comment:9 Changed 3 years ago by
Thank you François, it's a missing include. I will fix it in giacpy. It seems that between giac1.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
Commit:  85e78011637ab7fc553e3d1dd726311676412ec4 → 7a4bdb93fbf73c586b4e0cb8ee5235334b284d8b 

Branch pushed to git repo; I updated commit sha1. New commits:
7a4bdb9  update to giacpy_sage0.7.1 to fix a missing include with giac >1.5.0.75

comment:11 followup: 12 Changed 3 years ago by
Description:  modified (diff) 

the missing header with giac >1.5.0.75 reported by francois, should be fixed in giacpy_sage0.7.1 I have update the branch accordingly.
comment:12 Changed 3 years ago by
Replying to frederichan:
the missing header with giac >1.5.0.75 reported by francois, should be fixed in giacpy_sage0.7.1 I have update the branch accordingly.
Works now. Thanks!
comment:13 Changed 3 years ago by
Milestone:  sage9.1 → sage9.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:15 Changed 2 years ago by
Cc:  Vincent Delecroix added 

Priority:  major → critical 
comment:16 Changed 2 years ago by
Status:  new → needs_info 

comment:17 followup: 19 Changed 2 years ago by
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:
 build sagelib
 build giacpy_sage
 optional: test giacpy_sage
 build sagelib documentation
 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
Milestone:  sage9.2 → sageduplicate/invalid/wontfix 

Status:  needs_info → needs_review 
Yes, I agree that #29171, merging into sagelib, is the way to go.
comment:19 Changed 2 years ago by
comment:20 Changed 2 years ago by
Authors:  Frederic Han 

Reviewers:  → Markus Wageringel 
Status:  needs_review → positive_review 
comment:21 Changed 2 years ago by
Priority:  critical → major 

comment:22 Changed 2 years ago by
Description:  modified (diff) 

Resolution:  → invalid 
Status:  positive_review → closed 
What is required to make this happen?