Upgrade R to 3.4.0

[EmmanuelCharpentier]

Usual reasons (working on an u-to-date R is a prerequisite to get answers from the R Core Team on r-help...)

The new upstream version has been released almost a month ago.

Upstream tarball : https://cran.r-project.org/src/base/R-3/R-3.4.0.tar.gz

Depends on #23291

CC: @embray @jpflori

Component: packages: standard

Keywords: r-project

Author: Emmanuel Charpentier

Branch/Commit: 344d140

Reviewer: Erik Bray

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

[EmmanuelCharpentier]

Branch: u/charpent/R340

[EmmanuelCharpentier]

Author: Emmanuel Charpentier

[EmmanuelCharpentier]

Commit: 344d140

[EmmanuelCharpentier]

comment:2

Merged with #22989 (which needs_review, BTW), passes ptestlong with the usual transient failure sage -t --long src/sage/homology/simplicial_complex.py # 1 doctest failed, which passes when run standalone.

==>needs_review

---

New commits:

344d140

R 3.4.0 : dropped new source in place, fixed patches.

[EmmanuelCharpentier]

comment:3

Note : I am not sure that we are still justified to keep patches/libcurl_https_support.patch, which drops upstream's requirement of https support in libcurl.

This is unnecessary on Linux. I seem to remember that this was introduced to ease cygwin and/or OS/X compilation. Is that still the case ?

[EmmanuelCharpentier]

comment:4

Forgot to add : This R passes its own test suite (sage -f -c r succeeds, i. e. some failures are expected).

Note that in order to pass it, you HAVE to have en_GB.utf8 configured in your locales. This is also true of the original unpatched R. Note also that the test suite passes much more slowly in Sage than it does on the original unpatched R ; I do not yet know why.

[EmmanuelCharpentier]

comment:5

Wups ! Forgot to point to the upstream tarball... Fixed now.

[jpflori]

comment:6

Replying to @EmmanuelCharpentier:

Note : I am not sure that we are still justified to keep patches/libcurl_https_support.patch, which drops upstream's requirement of https support in libcurl.

This is unnecessary on Linux. I seem to remember that this was introduced to ease cygwin and/or OS/X compilation. Is that still the case ?

I guess so. Did anything change upstream?
I cannot see anything relevant in the changelog.

[EmmanuelCharpentier]

comment:7

This installs cleanly on Debian testing and on Ubuntu-over-WSL, but fails to install on cygwin. Reason nunuderstandable (the installation of the recommended packages "stats" seems to need under Cygwin a post-compilation step unneeded on Linux, which fails : rebasing needed ?)

==> needs_work

Help needed from people knowing cygwin better than I do.

Also : no news from the Maoc OS X front (I don't have that in my zoo...), and Apple's shenanigans with SSL have caused trouble for R in the past...

[embray]

comment:8

I'm having a look at the Cygwin build issue.

[embray]

comment:9

FWIW the failure I get is:

[r-3.4.0.p0] make[7]: Entering directory '/home/embray/src/sagemath/sage-cygwin/local/var/tmp/sage/build/r-3.4.0.p0/src/src/library/stats'
[r-3.4.0.p0] byte-compiling package 'stats'
[r-3.4.0.p0] Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) :
[r-3.4.0.p0]   object 'vI' not found
[r-3.4.0.p0] Calls: <Anonymous> ... namespaceImportFrom -> asNamespace -> loadNamespace
[r-3.4.0.p0] Execution halted
[r-3.4.0.p0] make[7]: *** [../../../share/make/lazycomp.mk:9: ../../../library/stats/R/stats.rdb] Error 1

No idea what any of this means. Still investigating.

[EmmanuelCharpentier]

comment:10

Replying to @embray:

FWIW the failure I get is:

[r-3.4.0.p0] make[7]: Entering directory '/home/embray/src/sagemath/sage-cygwin/local/var/tmp/sage/build/r-3.4.0.p0/src/src/library/stats'
[r-3.4.0.p0] byte-compiling package 'stats'
[r-3.4.0.p0] Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) :
[r-3.4.0.p0]   object 'vI' not found
[r-3.4.0.p0] Calls: <Anonymous> ... namespaceImportFrom -> asNamespace -> loadNamespace
[r-3.4.0.p0] Execution halted
[r-3.4.0.p0] make[7]: *** [../../../share/make/lazycomp.mk:9: ../../../library/stats/R/stats.rdb] Error 1

That's exactly the error I got (see the logs I sent ~ one week ago...).

No idea what any of this means.

Neither do I. ISTR that I thought it might be a rebasing problem, but I can't remember how I went to this guess, nor what was its rationale...

Still investigating.

Thank you very much : I'm out of my depth, here...

[embray]

comment:11

Well for starters there's a bug in R here. The object 'vI' not found is referring to a variable that is defined here, in a branch that is not being entered because the "package.rds" file, whatever that is, doesn't exist. There's even a comment that reads:

        ## Can we rely on the existence of R-ng 'nsInfo.rds' and
        ## 'package.rds'?
        ## No, not during builds of standard packages
        ## stats4 depends on methods, but exports do not matter

I don't fully know what that means, but we are in fact building a standard package (the "stats" package) so this file, apparently, isn't guaranteed to exist. The fact that later code uses a variable that isn't guaranteed to be defined is a bug.

Why this is being a problem on Cygwin and not on other platforms I don't know.

[embray]

comment:12

So on Linux I'm able to reproduce this bug by going into the built source, rm'ing src/library/stats/Meta/package.rds and then running tools:::makeLazyLoading("stats"). What's interesting is that on Linux the "package.rds" file does exist by this stage in the build process, whereas it does not on Cygwin. The comment I quoted that "package.rds" should not be relied on for builds of standard packages seems to contradict the current state of things.

Now just need to figure out where "package.rds" gets generated and see why that's not happening on Cygwin.

[embray]

comment:13

Well this escalated quickly. Turns out the "package.rds" files were not being written due to a segfault occurring during their processing, in the strsplit function. The following snippet reproduces: strsplit("Package", "\n[ \t\n]*\n", perl = TRUE). It doesn't segfault with perl = FALSE.

Even worse, for some reason R's SIGSEGV handler isn't working and it causes R to exit with a zero return code, indicating success. Weirdly, when I try to find out what was up with that, it seems like something is going horribly wrong in the signal handling. When I try to run it in gdb it seems like R's SIGSEGV handler is not being called at all. But when I run the process through strace, its SIGSEGV handler is called. So there's some shenanigans with the signal stack going on.

Finally, this doesn't break R's make because the target that generates these "package.rds" files is just a phony target called mkdesc, and the files it outputs are not specified anywhere as real prerequisites, so as far as make is concerned this rule succeeded an it proceeds under that assumption.

[EmmanuelCharpentier]

comment:14

Replying to @embray:

Well this escalated quickly. Turns out the "package.rds" files were not being written due to a segfault occurring during their processing, in the strsplit function. The following snippet reproduces: strsplit("Package", "\n[ \t\n]*\n", perl = TRUE). It doesn't segfault with perl = FALSE.

Does this happen
a. on Cygwin only, or
b. on Linux also ?

Even worse, for some reason R's SIGSEGV handler isn't working and it causes R to exit with a zero return code, indicating success. Weirdly, when I try to find out what was up with that, it seems like something is going horribly wrong in the signal handling. When I try to run it in gdb it seems like R's SIGSEGV handler is not being called at all. But when I run the process through strace, its SIGSEGV handler is called. So there's some shenanigans with the signal stack going on.

Same question : is this Cygwin-specific, or can it be reproduced on Linux (the successful compilation of R being just an happenstance) ?

Finally, this doesn't break R's make because the target that generates these "package.rds" files is just a phony target called mkdesc, and the files it outputs are not specified anywhere as real prerequisites, so as far as make is concerned this rule succeeded an it proceeds under that assumption.

This is a bug, notwithstanding it gets triggered on Linux or not. Don't you think that it should be reported upstream ?

[embray]

comment:15

I agree, the latter issue is a bug, as is the potentially undefined variable I mentioned earlier.

The other two issues seem to be Cygwin specific. R is using signaltstack for its SIGSEGV handling, which Cygwin does support, but I haven't seen too many programs that use this, so it could be buggy. That's sort of a tangential issue though--the real problem is the segfault itself, which I will investigate now.

[embray]

comment:16

Well I can't do anything more with this today. I've traced the segfault to somewhere deep inside the pcre_exec function. So I'm going to see if I can reproduce a simple example of this outside of R. That would give us a better idea if it was just an issue with rebase or not. I suspect not, because if were you wouldn't typically just get a segfault, but who knows. More likely, since Sage compiles its own libpcre, I wouldn't be surprised if there's a bug in there somewhere.

[embray]

comment:17

There's a patch from Cygwin that purports to fix the segfault in pcre. I'm trying it out now.

[embray]

comment:18

I should add, the bug causing the segfault makes such a mess of the stack that it probably explains why the segfault wasn't being handled properly either, though I haven't examined the specifics closely, and it's probably not worth the trouble to.

[embray]

Dependencies: #23291

[embray]

comment:19

Added #23291, which seems to fix the immediate build issue on Cygwin. Still needs to test that the package actually works.

[embray]

comment:20

Well, the R interpreter at least starts up fine and seems to work. That doesn't mean it's free of bugs, but the spkg-check for R is already known to have failures (#22866) so I think any other problems are outside the scope of this ticket, so long as it builds.

[jpflori]

comment:21

I support this statement.

[EmmanuelCharpentier]

comment:22

Do you (embray, jpflori) think that #23067+23291 is ready for review ? (BTW, #23291 is already at positive_review).

If so, I can do that tonight.

[embray]

comment:23

It looks good to me. I'm not sure what you're saying you'll do tonight though.

[embray]

Reviewer: Erik Bray

[EmmanuelCharpentier]

Changed reviewer from Erik Bray to none

[EmmanuelCharpentier]

comment:24

BTW : are you aware of a Mac user who might be solicited to test this on Mac OS X : Apple's shenanigans wit SLL (an, more generally, Xcode) has caused touble in the past...

Dima Pasechnik comes to mind, but seems awfukly busy nowadays...

Replying to @EmmanuelCharpentier:

Do you (embray, jpflori) think that #23067+23291 is ready for review ? (BTW, #23291 is already at positive_review).

If so, I can do that tonight.

[EmmanuelCharpentier]

Reviewer: Erik Bray

[EmmanuelCharpentier]

comment:25

Replying to @embray:

It looks good to me. I'm not sure what you're saying you'll do tonight though.

Just checking that starting from a fresh VM, I can obtain a working system (and install a few dozen R packages I happen to use more or less constantly).

But I agree that's just kicking the tyres... I also think that a similar tyre-kicking might be useful on Macs.

NB : the deletion of "Reviewer" has been done by Trac, not by me ! Setting it back.

[vbraun]

Changed branch from u/charpent/R340 to 344d140