Opened 17 months ago

Closed 8 months ago

#28918 closed enhancement (invalid)

Make giacpy_sage a standard package

Reported by: slelievre Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: packages: optional Keywords:
Cc: frederichan, gh-mwageringel, slelievre, vdelecroix 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 slelievre)

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 17 months ago by slelievre

  • Description modified (diff)
  • Milestone changed from sage-9.0 to sage-9.1

comment:2 follow-up: Changed 15 months ago by gh-mwageringel

What is required to make this happen?

comment:3 in reply to: ↑ 2 Changed 15 months ago by fbissey

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:5 Changed 15 months ago by frederichan

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 15 months ago by frederichan

  • Authors set to Frederic Han
  • Branch set to public/giacpy_sage-0.7
  • Commit set to 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 15 months ago by fbissey

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 15 months ago by fbissey (previous) (diff)

comment:8 Changed 15 months ago by fbissey

I actually have the same issue with giacpy_sage 0.6.8.

comment:9 Changed 15 months ago by frederichan

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 15 months ago by git

  • Commit changed from 85e78011637ab7fc553e3d1dd726311676412ec4 to 7a4bdb93fbf73c586b4e0cb8ee5235334b284d8b

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 follow-up: Changed 15 months ago by frederichan

  • 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 15 months ago by fbissey

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 13 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-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 10 months ago by mkoeppe

See also #29552

comment:15 Changed 9 months ago by mkoeppe

  • Cc vdelecroix added
  • Priority changed from major to critical

comment:16 Changed 9 months ago by mkoeppe

  • Status changed from new to needs_info

comment:17 follow-up: Changed 9 months ago by fbissey

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 9 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-duplicate/invalid/wontfix
  • Status changed from needs_info to needs_review

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

comment:19 in reply to: ↑ 17 Changed 9 months ago by mkoeppe

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 9 months ago by gh-mwageringel

  • Authors Frederic Han deleted
  • Reviewers set to Markus Wageringel
  • Status changed from needs_review to positive_review

comment:21 Changed 8 months ago by mkoeppe

  • Priority changed from critical to major

comment:22 Changed 8 months ago by slelievre

  • Description modified (diff)
  • Resolution set to invalid
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.