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

fix libgap on python3 #24990

Closed
dimpase opened this issue Mar 15, 2018 · 15 comments
Closed

fix libgap on python3 #24990

dimpase opened this issue Mar 15, 2018 · 15 comments

Comments

@dimpase
Copy link
Member

dimpase commented Mar 15, 2018

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.

Depends on #24343
Depends on #24922

CC: @fchapoton @embray @jdemeyer

Component: python3

Author: Dima Pasechnik

Branch/Commit: public/libgappy3 @ cd47b7e

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/24990

@dimpase dimpase added this to the sage-8.2 milestone Mar 15, 2018
@dimpase
Copy link
Member Author

dimpase commented Mar 15, 2018

comment:1

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

@dimpase
Copy link
Member Author

dimpase commented Mar 15, 2018

comment:2
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))

@dimpase
Copy link
Member Author

dimpase commented Mar 16, 2018

comment:3

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

@dimpase
Copy link
Member Author

dimpase commented Mar 16, 2018

Commit: cd47b7e

@dimpase
Copy link
Member Author

dimpase commented Mar 16, 2018

Branch: public/libgappy3

@dimpase
Copy link
Member Author

dimpase commented Mar 16, 2018

Dependencies: #24343, #24922

@dimpase
Copy link
Member Author

dimpase commented Mar 16, 2018

comment:5

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.

@fchapoton
Copy link
Contributor

comment:6

see also #24269 and its dependencies

@fchapoton
Copy link
Contributor

comment:7

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.

@embray
Copy link
Contributor

embray commented Mar 16, 2018

comment:8

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...

@embray
Copy link
Contributor

embray commented Mar 16, 2018

comment:9

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

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

@embray embray removed this from the sage-8.2 milestone Mar 16, 2018
@dimpase
Copy link
Member Author

dimpase commented Mar 16, 2018

Author: Dima Pasechnik

@dimpase
Copy link
Member Author

dimpase commented Mar 16, 2018

Reviewer: Erik Bray

@dimpase
Copy link
Member Author

dimpase commented Mar 16, 2018

comment:11

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.

@embray
Copy link
Contributor

embray commented Mar 16, 2018

comment:12

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

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

No branches or pull requests

3 participants