Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#22761 closed defect (fixed)

Update Sage patches to R; fix build on Cygwin

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.0
Component: packages: standard Keywords: cygwin windows r
Cc: jpflori, gouezel, tscrim Merged in:
Authors: Erik Bray Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 6fb42c2 (Commits, GitHub, GitLab) Commit:
Dependencies: #22627 Stopgaps:

Status badges

Description (last modified by embray)

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 13:ticket:22627). 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.

Change History (16)

comment:1 Changed 5 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 5 years ago by embray

  • Status changed from needs_review to needs_work

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.

Last edited 5 years ago by embray (previous) (diff)

comment:3 Changed 5 years ago by embray

  • Status changed from needs_work to needs_review

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

comment:4 Changed 5 years ago by embray

  • Status changed from needs_review to needs_work

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.

comment:5 Changed 5 years ago by git

  • Commit changed from 2337a62e85d8979c83db48423a2b4bc0c9d4b9f2 to 765373538db297f27359b9ad07a92088af8e59f1

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.

comment:6 Changed 5 years ago by embray

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

comment:7 Changed 5 years ago by git

  • Commit changed from 765373538db297f27359b9ad07a92088af8e59f1 to ed3fa8c7edd12c2e46ae4e96dae880931b8a4089

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

ed3fa8cUpdate R patch level

comment:8 Changed 5 years ago by embray

  • Dependencies set to #22627

New commits:

ed3fa8cUpdate R patch level

comment:9 Changed 5 years ago by tscrim

  • Cc jpflori gouezel tscrim added

comment:10 Changed 5 years ago by gouezel

  • Status changed from needs_review to needs_work

Does not apply

comment:11 Changed 5 years ago by git

  • Commit changed from ed3fa8c7edd12c2e46ae4e96dae880931b8a4089 to 6fb42c2737d21153348393bf3e16384369ea9f95

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

comment:12 Changed 5 years ago by embray

  • Status changed from needs_work to needs_review

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

comment:13 Changed 5 years ago by embray

  • Priority changed from major to blocker

comment:14 Changed 5 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

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

comment:15 Changed 5 years ago by vbraun

  • Branch changed from u/embray/cleanup-r-patches to 6fb42c2737d21153348393bf3e16384369ea9f95
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:16 Changed 5 years ago by embray

  • Commit 6fb42c2737d21153348393bf3e16384369ea9f95 deleted

Thanks!

Note: See TracTickets for help on using tickets.