Opened 9 years ago

Closed 7 years ago

Last modified 6 years ago

#12985 closed enhancement (fixed)

Build ECL with unicode enabled

Reported by: pcpa Owned by: was
Priority: minor Milestone: sage-6.4
Component: interfaces Keywords:
Cc: jpflori, Snark Merged in:
Authors: Paulo César Pereira de Andrade Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 9ae5068 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by pbruin)

The current ECL spkg disables unicode support, while it is enabled upstream.

New tarball: http://pub.math.leidenuniv.nl/~bruinpj/sage/ecl-12.12.1.20140917.tar.bz2

Attachments (5)

sage-5.0-ecl-unicode.patch (2.3 KB) - added by pcpa 9 years ago.
sage-5.0-ecl-unicode.patch
trac12985-ecl-unicode.patch (1.9 KB) - added by fbissey 9 years ago.
patch to wrap ecl potential unicode output
sage-5.2-ecl-unicode.patch (3.8 KB) - added by pcpa 9 years ago.
Rediff of patch and applying changes in nbruin comment
trac12985-unicode.patch (3.4 KB) - added by fbissey 9 years ago.
latest version of Paulo
trac_12985_ecl_unicode_5.10.beta4.patch (3.4 KB) - added by Snark 8 years ago.
Patch rebased for 5.10.beta4

Download all attachments as: .zip

Change History (53)

Changed 9 years ago by pcpa

sage-5.0-ecl-unicode.patch

comment:1 Changed 9 years ago by fbissey

Thanks Paulo, we had identified that problem in sage-on-gentoo a long time ago but since we can just ask for ecl to be build without unicode support for sage we just did that. If we apply your patch does sage work just as well if ecl is compiled without unicode support or does that become a requirement?

comment:2 follow-up: Changed 9 years ago by fbissey

Actually just looking at the patch, you are just adding a conversion layer, right?

comment:3 in reply to: ↑ 2 Changed 9 years ago by pcpa

Replying to fbissey:

Actually just looking at the patch, you are just adding a conversion layer, right?

Yes, and if the ecl is built without unicode support, it should be a minor overhead noop, as it would attempt to convert a base string to a base string :-) There are some comments from Juan at http://sourceforge.net/tracker/?func=detail&aid=3526370&group_id=30035&atid=398053 that may be of interest, about possible issues with unprotected lisp objects.

comment:4 Changed 9 years ago by nbruin

Juan's of course right in being concerned that objects are protected against garbage collection while alive. As far as I know, Boehm does watch the C stack for pointers, so a local variable pointing to the string should protect it against garbage collection while in scope.

I'm pretty sure that every ecl_base_string_pointer_safe occurs in a context where a python string object is required. Cython injects a PyBytes_FromString for that, which copies the content into a Python allocated object. The ECL-managed object is thus extremely short lived and during its life, is referenced by a local C variable (i.e., is referenced on the C stack). Boehm is supposed to watch the C stack for references, so it is protected against GC during its life.

If the reference returned by ecl_base_string_pointer_safe were to be stored in, say, a malloc'd location (or Python managed memory) such protection is no longer guaranteed. This is why EclObject.set_obj makes the little remove_node/insert_node_after dance, which ensures that a reference to the underlying ecl-object is stored within memory watched by Boehm-GC.

EDIT: Actually, I see what Juan is worried about: ecl_base_string_pointer_safe is a pointer to the string content, not the ecl string object. So yes, explicitly assigning the result of cl_write_to_string to a local variable, as he suggests, is probably a good idea.

The whole unicode problem would of course be much more elegantly solved by directly converting ecl unicode strings to python unicode strings and back. Given our use of ecl (maxima), there would be very little actual gain from doing so.

Last edited 9 years ago by nbruin (previous) (diff)

Changed 9 years ago by fbissey

patch to wrap ecl potential unicode output

comment:5 Changed 9 years ago by fbissey

  • Authors set to Paulo César Pereira de Andradet
  • Description modified (diff)
  • Reviewers set to François Bissey
  • Status changed from new to needs_review

I have put the patch in a better shape and checked it applied correctly on a recent 5.2beta0. I'd very much like this to go in 5.2. It doesn't really benefit sage at the present time but makes the works from people like me and Paulo on getting sage to run natively on linux distros much easier. I am doing a few tests and then I'll give it the thumbs up.

comment:6 follow-up: Changed 9 years ago by fbissey

Paulo I tried the patch with 5.1.beta6 and ecl-12.2.1 with unicode enabled and the sage/libs/ecl.pyx doctest stops waiting for input (with -verbose I get):

Trying:
    interrupt_after_delay(Integer(1000))###line 246:_sage_    >>> interrupt_after_delay(1000)
Expecting nothing
ok
Trying:
    inf_loop()###line 247:_sage_    >>> inf_loop()
Expecting:
    Traceback (most recent call last):
    ...
    RuntimeError: ECL says: Console interrupt

Condition of type: SIMPLE-TYPE-ERROR
In function MAKE-FOREIGN-DATA-FROM-ARRAY, the value of argument is
        "Console interrupt."
which is not of expected type BASE-STRING
Available restarts:

1. (USE-VALUE) Supply a new value of type BASE-STRING.

Top level.
> 

and it stays at that prompt until I interrupt it.

comment:7 Changed 9 years ago by pcpa

I cannot test it right now as I am still fighting to get a lot of packages updated in fedora, but will try to switch from sage-5.0.1 to 5.1.beta as soon as possible. The failure could be the requirement of some extra patch, or, the "interrupt after delay" may need some kind of "non interrruptible" code block when making the conversion.

But the error message appears to tell it somehow got a t_string where a t_base_string was expected, so, probably the ecl-unicode patch needs to be extended for 5.1.beta6.

Hopefully, sage/libs/ecl.pxd diff from 5.1.beta6 to 5.0.1 will show where the probem is, or, a new test case is triggering some unexpected condition.

comment:8 Changed 9 years ago by nbruin

Isn't this just a case of missed conversions? On line 256 we have

    if ecl_nvalues > 1:
        raise RuntimeError, "ECL says: "+ecl_base_string_pointer_safe(ecl_values(1))
    else:
        return ecl_values(0)

(this occurs in ecl_safe_eval, ecl_safe_funcall, ecl_safe_apply) and none of the patches here seem to touch that line. The second value will be ECL's error string, which is probably a unicode string if ECL uses unicode. Two solutions:

  • Change the definitions of the LISP routines sage-safe-eval and friend (line 212 and further) to already convert the error message to base_string, e.g.:
        (defun sage-safe-eval (form)
            (handler-case
                (values (eval form))
                (serious-condition (cnd)
                    (values nil (si:coerce-to-base-string (princ-to-string cnd))))))

I don't think in this case we need to store the reference to ecl_values(1), because the value itself is in a "last value returned" buffer by ecl and all we're doing is reaching into it to get a string pointer and copy it into a python string. No opportunity for ecl to invalidate the values buffer.

  • do the conversion in the python routines:
cdef cl_object ecl_safe_eval(cl_object form) except NULL:
    cdef cl_object s 
    ecl_sig_on()
    cl_funcall(2,safe_eval_clobj,form)
    ecl_sig_off()

    if ecl_nvalues > 1:
        s = si_coerce_to_base_string(ecl_values(1))
        raise RuntimeError, "ECL says: "+ecl_base_string_pointer_safe(s)
    else:
        return ecl_values(0)

Changed 9 years ago by pcpa

Rediff of patch and applying changes in nbruin comment

comment:9 Changed 9 years ago by jpflori

  • Cc jpflori added

comment:10 in reply to: ↑ 6 Changed 9 years ago by fbissey

Replying to fbissey:

Paulo I tried the patch with 5.1.beta6 and ecl-12.2.1 with unicode enabled and the sage/libs/ecl.pyx doctest stops waiting for input (with -verbose I get):

Trying:
    interrupt_after_delay(Integer(1000))###line 246:_sage_    >>> interrupt_after_delay(1000)
Expecting nothing
ok
Trying:
    inf_loop()###line 247:_sage_    >>> inf_loop()
Expecting:
    Traceback (most recent call last):
    ...
    RuntimeError: ECL says: Console interrupt

Condition of type: SIMPLE-TYPE-ERROR
In function MAKE-FOREIGN-DATA-FROM-ARRAY, the value of argument is
        "Console interrupt."
which is not of expected type BASE-STRING
Available restarts:

1. (USE-VALUE) Supply a new value of type BASE-STRING.

Top level.
> 

and it stays at that prompt until I interrupt it.

With ecl-12.7.1 it just hangs at the same spot even without unicode so there may be something deeper going on there. Haven't tried the new patch it did go under my radar unnoticed so I don't know if it will solve the problem yet.

comment:11 Changed 9 years ago by fbissey

I will have to reformat your patch a little bit Paulo. Not only it wasn't produced by hg my cython didn't like the result very much

Error compiling Cython file:
------------------------------------------------------------
...
    ecl_sig_on()
    cl_funcall(2,safe_eval_clobj,form)
    ecl_sig_off()

    if ecl_nvalues > 1:
        s = si_coerce_to_base_string(ecl_values(1))
^
------------------------------------------------------------

sage/libs/ecl.pyx:258:0: Mixed use of tabs and spaces

comment:12 Changed 9 years ago by fbissey

I have a riddle. With the patch and ecl 12.7.1

sage -t -long -force_lib "devel/sage/sage/libs/ecl.pyx" -verbose

take 2.9s and is successful if you exclude this little problem but if I run it without "-verbose" it times out - presumably at the same spot. And I haven't tried unicode yet.

Last edited 9 years ago by fbissey (previous) (diff)

comment:13 Changed 9 years ago by pcpa

I noticed later I submitted a patch with tabs instead of spaces in a few places but left it as an exercise for the reader to correct :-)

It appears to work in my current Fedora build

$ rpm -q sagemath ecl
sagemath-5.2-1.fc18.x86_64
ecl-12.2.1-4.fc18.x86_64

Running it I see:

$ sage -t -long -force_lib "devel/sage/sage/libs/ecl.pyx"
sage -t -long -force_lib "devel/sage/sage/libs/ecl.pyx"     
**********************************************************************
File "/usr/share/sagemath/devel/sage/sage/libs/ecl.pyx", line 247:
    sage: inf_loop()
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: ECL says: Console interrupt
Got:
    Traceback (most recent call last):
      File "/usr/share/sagemath/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/usr/share/sagemath/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/usr/share/sagemath/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_6[7]>", line 1, in <module>
        inf_loop()###line 247:
    sage: inf_loop()
      File "ecl.pyx", line 712, in sage.libs.ecl.EclObject.__call__ (sage/libs/ecl.c:5042)
      File "ecl.pyx", line 285, in sage.libs.ecl.ecl_safe_apply (sage/libs/ecl.c:3047)
    RuntimeError: ECL says: Console interrupt.
**********************************************************************
1 items had failures:
   1 of   9 in __main__.example_6
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/pcpa/.sage/tmp/ecl_1962.py
         [7.1 s]
 
----------------------------------------------------------------------
The following tests failed:


        sage -t -long -force_lib "devel/sage/sage/libs/ecl.pyx"
Total time for all tests: 7.2 seconds

Also takes 2.8s in the second run...

I suspect it should be ecl-2.7.1, but there may be a few other variables involving gc and glibc version.

I did not look at ecl-2.7.1 yet, but maybe need a patch equivalent to

http://pkgs.fedoraproject.org/cgit/ecl.git/tree/ecl-12.2.1-signal_handling_thread.patch

Should not be required as sagemath should already have an equivalent patch, and the above patch was required to make maxima ecl backend not hang.

comment:14 Changed 9 years ago by fbissey

We don't that patch in Gentoo at all. Never noticed a problem with maxima so far. It is just curious that with 12.7.1 I get a different behavior between a verbose and non-verbose test. No that's a puzzler to me. There must be a variable affecting ecl or something.

comment:15 follow-up: Changed 9 years ago by fbissey

OK your patch now works as expected without fuss with ecl-12.2.1. I'll reformat the patch for inclusion in sage and save the weird behavior in 12.7.1 for another day. Sounds OK?

Changed 9 years ago by fbissey

latest version of Paulo

comment:16 Changed 9 years ago by fbissey

  • Description modified (diff)
  • Status changed from needs_review to positive_review

Re-generated the patch against sage-5.3.beta0. Pushing the positive review button.

comment:17 in reply to: ↑ 15 Changed 9 years ago by pcpa

Replying to fbissey:

OK your patch now works as expected without fuss with ecl-12.2.1. I'll reformat the patch for inclusion in sage and save the weird behavior in 12.7.1 for another day. Sounds OK?

It is ok with me. I should test latest upstream ecl at some point also. BTW, the ecl-12.2.1-signal_handling_thread.patch in Fedora was my solution to allow maxima's make check to work with the ecl backend, when I made patches and requested support for the ecl backend; most tests would work, I think it was that it would fail somewhere in the tests, probably due to triggering gc in some specific time window.

comment:18 follow-up: Changed 9 years ago by fbissey

  • Status changed from positive_review to needs_info

Wait a minute we cannot merge this if we don't have ecl compiled with unicode.

comment:19 in reply to: ↑ 18 Changed 9 years ago by pcpa

Replying to fbissey:

Wait a minute we cannot merge this if we don't have ecl compiled with unicode.

It is supposed to be a noop if ecl is built without unicode support. AFAIR I made tests with unicode disabled and applying the propsed patch, but it should be wise to test it again.

comment:20 Changed 9 years ago by fbissey

It causes problem for me with

sage -t -long  -force_lib devel/sage-main/sage/calculus/calculus.py

[?1034h#0: solve_rat_ineq(ineq=x # 5)
#0: simplify_sum(expr='sum(q^k,k,0,inf))
#1: simplify_sum(expr=a*'sum(q^k,k,0,inf))
**********************************************************************
File "/usr/share/sage/devel/sage-main/sage/calculus/calculus.py", line 677:
    sage: f.nintegral(x,0,1,1e-14)
Exception raised:
    Traceback (most recent call last):
      File "/usr/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/usr/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/usr/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_3[3]>", line 1, in <module>
        f.nintegral(x,Integer(0),Integer(1),RealNumber('1e-14'))###line 677:
    sage: f.nintegral(x,0,1,1e-14)
      File "expression.pyx", line 9073, in sage.symbolic.expression.Expression.nintegral (sage/symbolic/expression.cpp:38115)
      File "/usr/lib64/python2.7/site-packages/sage/calculus/calculus.py", line 752, in nintegral
        raise ValueError, "Maxima (via quadpack) cannot compute the integral"
    ValueError: Maxima (via quadpack) cannot compute the integral
**********************************************************************
1 items had failures:
   1 of  17 in __main__.example_3
***Test Failed*** 1 failures.

May be your maxima patch deals with that. If I use an ecl compiled without unicode and this patch at the same time we there are unresolved symbol because of the declaration

cl_object si_coerce_to_base_string(cl_object x) 

in ecl.pxd. It won't be there if ecl is compiled without unicode. I may have to crosscheck that if that particular bit is nasty or not.

comment:21 Changed 9 years ago by pcpa

Sage and maxima must link to an ecl with the same build options. From /usr/include/ecl/external.h

#ifdef ECL_UNICODE
extern ECL_API cl_object si_base_char_p(cl_object x);
extern ECL_API cl_object si_base_string_p(cl_object x);
extern ECL_API cl_object si_coerce_to_base_string(cl_object x);
extern ECL_API cl_object si_coerce_to_extended_string(cl_object x);
#define ecl_alloc_simple_extended_string(l) ecl_alloc_simple_vector((l),aet_ch)
extern ECL_API cl_object ecl_alloc_adjustable_extended_string(cl_index l);
#else
#define si_base_char_p cl_characterp
#define si_base_string_p cl_stringp
#define si_coerce_to_base_string cl_string
#define si_coerce_to_extended_string cl_string
#endif

comment:22 Changed 9 years ago by fbissey

That would be why I got bad stuff about unresolved symbols with ecl.pyx but for the calculus.py doctest I had something consistent: ecl with unicode and maxima and sage rebuilt against it.

comment:23 Changed 8 years ago by kcrisman

  • Description modified (diff)

comment:24 follow-up: Changed 8 years ago by kcrisman

The current packages at #13324 have

--enable-unicode=no

Would that be an acceptable resolution to this ticket, or should we re-enable it with this fix eventually? (Currently I'd say #13324 is higher-priority so that we might actually have a chance at getting Cygwin up to speed...)

comment:25 in reply to: ↑ 24 Changed 8 years ago by pcpa

Replying to kcrisman:

The current packages at #13324 have --enable-unicode=no

The fedora package has an explicit --enable-unicode. The fact that ecl was failing make check if using --disable-unicode was an ecl bug.

Would that be an acceptable resolution to this ticket, or should we re-enable it with this fix eventually? (Currently I'd say #13324 is higher-priority so that we might actually have a chance at getting Cygwin up to speed...)

The patch must work with ecl compiled with or without unicode (the patch reason is to not break sagemath if ecl was built with unicode enabled), but the patch was causing a link failure if using an ecl older than 12.2.

comment:26 Changed 8 years ago by fbissey

I'll have to check if I still have a couple of weird issues stemming from Unicode and this patch. Haven't touched it in while. I have been more focused on upgrading numpy in #11334 of late.

Changed 8 years ago by Snark

Patch rebased for 5.10.beta4

comment:27 Changed 8 years ago by Snark

  • Cc Snark added

comment:28 Changed 8 years ago by Snark

I checked that the rebased patch used with the current ECL spkg which disables unicode, works: everything compiles and doctests.

comment:29 Changed 8 years ago by Snark

  • Description modified (diff)
  • Status changed from needs_info to needs_review
  • Summary changed from Possible future issues with ECL build with unicode enabled to Build ECL with unicode enabled

comment:30 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:31 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:32 Changed 7 years ago by vbraun

  • Description modified (diff)

comment:33 Changed 7 years ago by vbraun

  • Branch set to u/vbraun/build_ecl_with_unicode_enabled

comment:34 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:35 Changed 7 years ago by pbruin

  • Commit set to 7d358ff15e482ef45c54222119577cf84fd07c41
  • Status changed from needs_review to needs_work
  • does not merge with 6.3.beta2
  • probably needs a new tarball

New commits:

ed10dceTrac 12985: make sage work with ecl compiled with unicode support
7d358ffenable unicode in ecl
Last edited 7 years ago by pbruin (previous) (diff)

comment:36 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:37 Changed 7 years ago by pbruin

  • Branch changed from u/vbraun/build_ecl_with_unicode_enabled to u/pbruin/12985-ecl_unicode
  • Commit changed from 7d358ff15e482ef45c54222119577cf84fd07c41 to 9ae50681bb70ed16c046e296fe72dbc1c43f8eb3
  • Description modified (diff)
  • Report Upstream changed from Reported upstream. Developers deny it's a bug. to N/A
  • Status changed from needs_work to needs_review

Merged with 6.4.beta3, updated tarball, minor changes to files in build/pkgs/ecl/.

comment:38 follow-up: Changed 7 years ago by jpflori

Is there any chance to update to the latest (and final?) ECL? Wasn't the main blocker the previous versions of Maxima?

comment:39 in reply to: ↑ 38 Changed 7 years ago by pbruin

Replying to jpflori:

Is there any chance to update to the latest (and final?) ECL? Wasn't the main blocker the previous versions of Maxima?

I think we should give it a try, but on a separate ticket...

comment:40 Changed 7 years ago by jpflori

Sure, on another ticket :)

comment:41 follow-up: Changed 7 years ago by fbissey

Do we have one? We are now fully using ecl 13.5.1 and there are no problems with it. So this will really be the last ecl for potentially a long time? One of the reason we switched to ecl was the "maxima as a library" thing. Now that particular extension has been included in asdf 3+ so we could potentially build it with another lisp.

comment:42 follow-up: Changed 7 years ago by fbissey

I meant do we have a ticket and sage-on-gentoo is using 13.5.1.

comment:43 in reply to: ↑ 42 ; follow-up: Changed 7 years ago by pbruin

Replying to fbissey:

I meant do we have a ticket and sage-on-gentoo is using 13.5.1.

We don't seem to have a ticket yet; I'll open one.

comment:44 in reply to: ↑ 43 Changed 7 years ago by pbruin

Replying to pbruin:

Replying to fbissey:

I meant do we have a ticket and sage-on-gentoo is using 13.5.1.

We don't seem to have a ticket yet; I'll open one.

This is now #17003.

comment:45 in reply to: ↑ 41 Changed 7 years ago by nbruin

Replying to fbissey:

One of the reason we switched to ecl was the "maxima as a library" thing. Now that particular extension has been included in asdf 3+ so we could potentially build it with another lisp.

Before you can use "maxima as a library" from sage, you first need "lisp as a library" and I think ECL is still the only player in that arena. I'm sure you can get "maxima as a lisp library" in pretty much any lisp implementation, but we need to be able to call into that lisp implementation from C code.

comment:46 Changed 7 years ago by fbissey

  • Status changed from needs_review to positive_review

OK, it passes basic review here but maxima and sage need rebuilding. I assume that will be automagic with #17072.

comment:47 Changed 7 years ago by vbraun

  • Branch changed from u/pbruin/12985-ecl_unicode to 9ae50681bb70ed16c046e296fe72dbc1c43f8eb3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:48 Changed 6 years ago by jdemeyer

  • Authors changed from Paulo César Pereira de Andradet to Paulo César Pereira de Andrade
  • Commit 9ae50681bb70ed16c046e296fe72dbc1c43f8eb3 deleted
Note: See TracTickets for help on using tickets.