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

Update Sage patches to R; fix build on Cygwin #22761

Closed
embray opened this issue Apr 5, 2017 · 19 comments
Closed

Update Sage patches to R; fix build on Cygwin #22761

embray opened this issue Apr 5, 2017 · 19 comments

Comments

@embray
Copy link
Contributor

embray commented Apr 5, 2017

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 embray added this to the sage-8.0 milestone Apr 5, 2017
@embray
Copy link
Contributor Author

embray commented Apr 5, 2017

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
Copy link
Contributor Author

embray commented Apr 5, 2017

comment:3

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

@embray
Copy link
Contributor Author

embray commented Apr 5, 2017

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2017

Changed commit from 2337a62 to 7653735

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2017

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

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

@embray

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2017

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

ed3fa8cUpdate R patch level

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2017

Changed commit from 7653735 to ed3fa8c

@embray
Copy link
Contributor Author

embray commented Apr 5, 2017

Dependencies: #22627

@embray
Copy link
Contributor Author

embray commented Apr 5, 2017

New commits:

ed3fa8cUpdate R patch level

@sagetrac-gouezel
Copy link
Mannequin

sagetrac-gouezel mannequin commented Apr 8, 2017

comment:10

Does not apply

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2017

Changed commit from ed3fa8c to 6fb42c2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2017

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

84ab84aUpdated this patch not to mess with R_HOME_DIR if SAGE_LOCAL is *not* set.
0f4f542Added some basic comments to this patch
1a02d2bUpdated this patch to also include the actual resulting updates to the configure script
5e9c424Removed the old cygwin build patches that were not being applied.
6fb42c2Update R patch level

@embray
Copy link
Contributor Author

embray commented Apr 10, 2017

comment:12

I think there was some weird disconnect between when I made this branch, and when #22627 was actually merged into develop...

@tscrim
Copy link
Collaborator

tscrim commented Apr 21, 2017

comment:14

Built for me okay on Cygwin. Off to the buildbots.

@tscrim
Copy link
Collaborator

tscrim commented Apr 21, 2017

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Apr 23, 2017

Changed branch from u/embray/cleanup-r-patches to 6fb42c2

@embray
Copy link
Contributor Author

embray commented Apr 24, 2017

comment:16

Thanks!

@embray
Copy link
Contributor Author

embray commented Apr 24, 2017

Changed commit from 6fb42c2 to none

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