Bug 15660

Summary: Enable support for using system tre
Product: R Reporter: Tom Callaway <tcallawa>
Component: System-specificAssignee: R-core <R-core>
Status: CLOSED FIXED    
Severity: enhancement CC: orion, simon.urbanek
Priority: P5    
Version: R 3.0.2   
Hardware: Other   
OS: Linux   
Attachments: Enable --with-system-tre
[1 of 2] Changes R made to stock tre
[2 of 2] Changes R made to stock tre

Description Tom Callaway 2014-02-07 21:09:54 UTC
Created attachment 1559 [details]
Enable --with-system-tre

Recently, Fedora enabled armv7 as a primary architecture for all builds. We noticed that R builds on that target were succeeding, but were not working properly, specifically that it could not load its own built-in datasets.

More research uncovered that this was a known issue, Debian found it here:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=695411

They noticed it was functionally identical to an HP-UX issue, filed in R bugzilla here:

https://bugs.r-project.org/bugzilla/show_bug.cgi?id=15087

Orion Poplawski then was able to determine that the issue was with the bundled tre library that R includes. When we applied the R specific changes to the Fedora system tre library (attached to this bug as tre-0.8.0-Rfixes-8e84229.patch and tre-0.8.0-Rfixes-d0a2382.patch), then linked R to that tre shared library, this bug went away on arm.

This indicates that there is a need to fix this bug in your bundled copy of tre, however, Fedora would prefer to simply use our fixed system version of tre (and are already doing so). I've attached a copy of a patch to enable building against a system version of tre as a configuration option. It is disabled by default, and checks that the tre library has tre_regncompb (this function is not present in stock tre at the moment, only in the R changes to tre). You'll need to rerun autotools for this change to fully enable.

We've also submitted these two R changesets to tre to the upstream tre team, and we're hopeful they will be accepted.
Comment 1 Tom Callaway 2014-02-07 21:10:36 UTC
Created attachment 1560 [details]
[1 of 2] Changes R made to stock tre
Comment 2 Tom Callaway 2014-02-07 21:10:59 UTC
Created attachment 1561 [details]
[2 of 2] Changes R made to stock tre
Comment 3 Orion Poplawski 2014-02-28 18:44:23 UTC
FYI - Upstream tre has now merged in a minimal set of R changes to the tre library to allow using it: https://github.com/laurikari/tre/pull/14

If there are any other R changes to the tre library that are beneficial, please submit them upstream.
Comment 4 Brian Ripley 2014-03-02 14:13:20 UTC
This came too late for the 3.0.x branch and is the sort of thing we would only do for a 3.x.0 release.

We have started enabling it for 3.1.0 (due April 10).
Comment 5 Brian Ripley 2014-03-02 19:39:57 UTC
Note however, that building a tre from the current github sources results in an R which fails its tests at

running regexp regression tests
make[2]: *** [test-Regexp] Error 1

> ## this broke and segfaulted in 2.13.1 and earlier (PR#14627)
> x <- paste(rep("a ", 600), collapse="")
> testit(agrep(x, x))
Error: segfault from C stack overflow

So we cannot yet endorse this.
Comment 6 Orion Poplawski 2014-03-03 22:49:26 UTC
I did not send the R changes to tre to make it use a stack rather than recursion (which fixes the this crash) upstream.  I just made a quick attempt to distil those changes into a separate patch, but I was unsuccessful (it broke tre), so I suspect it relied on other changes R made to tre.

Would it be possible for the R and tre developers to get together to get other needed changes merged into the upstream tre library (hopefully without breaking it on arm)?  I'm afraid I'm a pretty poor go between at this point.   Simon Urbanek was the original author of the tre stack change, perhaps he can help?