Update Sage patches to R; fix build on Cygwin

embray commented 7 years ago

I regenerated some of the patches we maintain for R, from a repository based on R 3.3.3, the current version in Sage since #20523. So the new patches should apply more cleanly. I also updated a few of the patches slightly:

Updated hardcoded_dirs.patch so that it only takes action if $SAGE_LOCAL is actually set. Otherwise it would set an invalid $R_HOME_DIR. This was confusing for testing the validity of the patch set outside the context of Sage.
The original m4_macro_bug.patch only patched the m4 script, but did not update the configure script with the resulting changes.
The patches for Cygwin were not being applied (see [#22627 comment:13]). This fixes that, but also reworks the original cygwin.patch in such a way that it doesn't break building on other platforms. Instead of trying to make a DLL import lib (libR.dll.a) this relies on the fact that direct linking (see under "direct linking to a dll") to libR.dll was already working. And in fact trying to use the import lib didn't work immediately without patching rpy2.

The reason it wasn't working seems to be some undocumented (that I can find) subtlety with symbol resolution in ld when direct linking to a DLL versus using an import lib. For some reason it's less fussy about -l flag order when using direct linking. At any case, not using an import lib for R works in this case and simplifies the patches needed for Cygwin support.

This fixes building R 3.3.3 on Cygwin.

Depends on #22627

CC: @jpflori @sagetrac-gouezel @tscrim

Component: packages: standard

Keywords: cygwin windows r

Author: Erik Bray

Branch: 6fb42c2

Reviewer: Travis Scrimshaw

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

embray commented 7 years ago

comment:2

Looks like the Cygwin aspect of this still needs work, as building rpy2 demonstrates for me. I'm not sure libR.dll is being installed in the correct place.

embray commented 7 years ago

comment:3

Nevermind, this seems to have more to do with rpy2 I think.

embray commented 7 years ago

comment:4

Sorry, I think it is this after all. I can't reproduce the problem on an older branch, and there were no changes to rpy2. I think it's probably an installation path issue with the new build changes to R.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 2337a62 to 7653735

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7653735 Removed the old cygwin build patches that were not being applied.

embray commented 7 years ago

Description changed:

--- 
+++ 
@@ -4,6 +4,8 @@

 * The original `m4_macro_bug.patch` only patched the m4 script, but did not update the `configure` script with the resulting changes.

-* The patches for Cygwin were not being applied (see [[#22627 comment:13](https://github.com/sagemath/sage/issues/22627#comment:13)]).  This fixes that, but also reworks the original `cygwin.patch` in such a way that it doesn't break building on other platforms.  Instead it includes new logic for handling a DLL import library (referred to as `LDLIB`) that is only used where relevant (I think this would useful to MinGW as well but we're not worrying about that right now).
+* The patches for Cygwin were not being applied (see [[#22627 comment:13](https://github.com/sagemath/sage/issues/22627#comment:13)]).  This fixes that, but also reworks the original `cygwin.patch` in such a way that it doesn't break building on other platforms.  Instead of trying to make a DLL import lib (`libR.dll.a`) this relies on the fact that [direct linking](https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/Using_ld_the_GNU_Linker/win32.html) (see under "direct linking to a dll") to `libR.dll` was already working.  And in fact trying to use the import lib *didn't* work immediately without patching rpy2.
+
+The reason it wasn't working seems to be some undocumented (that I can find) subtlety with symbol resolution in `ld` when direct linking to a DLL versus using an import lib.  For some reason it's less fussy about `-l` flag order when using direct linking.  At any case, not using an import lib for R works in this case and simplifies the patches needed for Cygwin support.

 This fixes building R 3.3.3 on Cygwin.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

ed3fa8c Update R patch level

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 7653735 to ed3fa8c

embray commented 7 years ago

Dependencies: #22627

embray commented 7 years ago

New commits:

ed3fa8c Update R patch level

85c38e47-45b8-4df2-bb1e-fcf35b47c1e0 commented 7 years ago

comment:10

Does not apply

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from ed3fa8c to 6fb42c2

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`84ab84a`	`Updated this patch not to mess with R_HOME_DIR if SAGE_LOCAL is not set.`
`0f4f542`	`Added some basic comments to this patch`
`1a02d2b`	`Updated this patch to also include the actual resulting updates to the configure script`
`5e9c424`	`Removed the old cygwin build patches that were not being applied.`
`6fb42c2`	`Update R patch level`