Opened 3 years ago

Closed 3 years ago

#24990 closed defect (duplicate)

fix libgap on python3

Reported by: gh-dimpase Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: python3 Keywords:
Cc: chapoton, embray, jdemeyer Merged in:
Authors: Dima Pasechnik Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: public/libgappy3 (Commits, GitHub, GitLab) Commit: cd47b7e16f06ad84d8959ee9bb458fc6ce4a7ab2
Dependencies: #24343, #24922 Stopgaps:

Status badges

Description

cython (cdef) function initialize() fails in src/sage/libs/gap/util.pyx with TypeError: expected bytes, str found.

This can be traced to the lines

    from sage.interfaces.gap import _get_gap_memory_pool_size_MB
    memory_pool = _get_gap_memory_pool_size_MB()
    argv[3] = "-o"
    argv[4] = memory_pool

which get a string memory_pool from Python and try to assign it to argv[4] of C type char*. This is something that needs extra care on Python3, as memory_pool is of type str.

Change History (13)

comment:1 Changed 3 years ago by gh-dimpase

I believe this kind of thing has been fixed already in other places.

comment:2 Changed 3 years ago by gh-dimpase

sage: libgap(1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-037524c8c625> in <module>()
----> 1 libgap(Integer(1))

/mnt/opt/Sage/sage-clang/local/lib/python3.6/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3706)()

/mnt/opt/Sage/sage-clang/local/lib/python3.6/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.get_object (build/cythonized/sage/misc/lazy_import.c:2210)()

/mnt/opt/Sage/sage-clang/local/lib/python3.6/site-packages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport._get_object (build/cythonized/sage/misc/lazy_import.c:2480)()

/mnt/opt/Sage/sage-clang/local/lib/python3.6/site-packages/sage/libs/gap/libgap.pyx in init sage.libs.gap.libgap()
    785 
    786 
--> 787 libgap = Gap()

/mnt/opt/Sage/sage-clang/local/lib/python3.6/site-packages/sage/libs/gap/libgap.pyx in sage.libs.gap.libgap.Gap.__init__ (build/cythonized/sage/libs/gap/libgap.c:5521)()
    615             <class 'sage.libs.gap.libgap.Gap'>
    616         """
--> 617         initialize()
    618         libgap_set_gasman_callback(gasman_callback)
    619         from sage.rings.integer_ring import ZZ

/mnt/opt/Sage/sage-clang/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.initialize (build/cythonized/sage/libs/gap/util.c:5210)()

/mnt/opt/Sage/sage-clang/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.initialize (build/cythonized/sage/libs/gap/util.c:5166)()

/mnt/opt/Sage/sage-clang/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:5399)()

TypeError: expected bytes, str found

This is an error in here, lines 246-8 of sage/libs/gap/util.pyx

       with atomic_write(workspace) as f:             
            f.close()
            gap_eval('SaveWorkspace("{0}")'.format(f.name))
Last edited 3 years ago by gh-dimpase (previous) (diff)

comment:3 Changed 3 years ago by gh-dimpase

  • Branch set to public/libgappy3
  • Commit set to cd47b7e16f06ad84d8959ee9bb458fc6ce4a7ab2

these are few changes that I was able to figure out so far.


New commits:

af86e5cVarious fixes intended to ensure that opened files are closed explictly where
436ed26A few other miscellaneous fixes to the doctests tests
2155133missing string encoding fix
d3922d7Merge branch 'u/embray/python3/sage-doctest' of trac.sagemath.org:sage into develop
e566901Use the restore_atexit context manager for this code to work on Python 3,
6fd2912Add run option to restore_atexit context
46017b6Use restore_atexit(run=True) in doctest framework
13f433cmake this exception message more flexible to the exact point in the Python interpreter where it occurred
1b64674Merge branch 'u/embray/use_restore_atexit_in_doctest_framework' of trac.sagemath.org:sage into develop
cd47b7e1st changes needed to deal with str/byte thing

comment:4 Changed 3 years ago by gh-dimpase

  • Dependencies set to #24343, #24922

comment:5 Changed 3 years ago by gh-dimpase

after

diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx
index b224e4d3bd..851966c146 100644
--- a/src/sage/libs/gap/util.pyx
+++ b/src/sage/libs/gap/util.pyx
@@ -299,7 +299,7 @@ cdef libGAP_Obj gap_eval(str gap_string) except? NULL:
     initialize()
     cdef libGAP_ExecStatus status
 
-    cmd = gap_string + ';\n'
+    cmd = bytes(gap_string + ';\n', encoding="utf-8")
     try:
         try:
             sig_on()

I get past initialization(), and can do things like libgap.eval("blah"), but get a segfault on libgap(1) (but not on libgap("1")).

It seems to happen at return libGAP_IS_FUNC(self.value), line 1024 of sage/libs/gap/element.pyx.

comment:6 Changed 3 years ago by chapoton

see also #24269 and its dependencies

comment:7 Changed 3 years ago by chapoton

  • Cc embray jdemeyer added

Wrapping with bytes() is not the correct way to go ; one should rather use str_to_bytes or the converse bytes_to_str, see other py3 tickets.

And cython understands basestring, no need to import it from anywhere.

comment:8 Changed 3 years ago by embray

I already fixed all the issues with sage.libs.gap. I'm pretty sure there's already a ticket for it but let me make sure...

comment:9 Changed 3 years ago by embray

  • Milestone changed from sage-8.2 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

Yeah, it's here, but I need to rebase it: #24460

I propose to close this ticket as a duplicate if that's alright.

comment:10 Changed 3 years ago by gh-dimpase

  • Authors set to Dima Pasechnik
  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

comment:11 Changed 3 years ago by gh-dimpase

This is fine. I opened it cause I missed #24460 in my search (I looked for libgap :/) and I needed libgap to work to be able to properly do the work on #24984, which depends on libgap a lot.

comment:12 Changed 3 years ago by embray

Ah, I should put libgap in the keywords as well.

comment:13 Changed 3 years ago by embray

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.