cygwin: Do not use system R

mkoeppe commented 4 years ago

For package rpy2 (versions 2.8.x) there is an unresolved build failure on cygwin when using system R (cygwin-standard). This can be seen on the GitHub Actions workflow from #29403 but has also been reproduced locally. Details below.

This ticket disables use of system R on cygwin. This should be reverted with the #29441 upgrade to more recent rpy2 (python-3 only).

gcc -shared -Wl,--enable-auto-image-base -L/cygdrive/d/a/sage/sage/local/lib -Wl,-rpath,/cygdrive/d/a/sage/sage/local/lib build/temp.cygwin-3.1.4-x86_64-3.7/./rpy/rinterface/_rinterface.o -L/usr/lib/R/lib -L/cygdrive/d/a/sage/sage/local/lib/python3.7/config -L/usr/lib -Lbuild/temp.cygwin-3.1.4-x86_64-3.7 -L/usr/lib/R/lib -lreadline -lR -lpcre -llzma -lbz2 -lz -ltirpc -lrt -ldl -lm -liconv -licuuc -licui18n -lpython3.7m -lr_utils -o build/lib.cygwin-3.1.4-x86_64-3.7/rpy2/rinterface/_rinterface.cpython-37m-x86_64-cygwin.dll -fopenmp
    /usr/lib/gcc/x86_64-pc-cygwin/9.3.0/../../../../x86_64-pc-cygwin/bin/ld: build/temp.cygwin-3.1.4-x86_64-3.7/libr_utils.a(r_utils.o): in function `rpy2_findfun':
    /tmp/pip-req-build-ymc5_3kd/./rpy/rinterface/r_utils.c:58: undefined reference to `Rf_findVarInFrame3'
    /tmp/pip-req-build-ymc5_3kd/./rpy/rinterface/r_utils.c:58:(.text+0xda): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `Rf_findVarInFrame3'
    /usr/lib/gcc/x86_64-pc-cygwin/9.3.0/../../../../x86_64-pc-cygwin/bin/ld: build/temp.cygwin-3.1.4-x86_64-3.7/libr_utils.a(r_utils.o): in function `rpy2_list_attr':
    /tmp/pip-req-build-ymc5_3kd/./rpy/rinterface/r_utils.c:129: undefined reference to `ATTRIB'
    /tmp/pip-req-build-ymc5_3kd/./rpy/rinterface/r_utils.c:129:(.text+0x340): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `ATTRIB'
    /usr/lib/gcc/x86_64-pc-cygwin/9.3.0/../../../../x86_64-pc-cygwin/bin/ld: build/temp.cygwin-3.1.4-x86_64-3.7/libr_utils.a(r_utils.o): in function `rpy2_remove':
    /tmp/pip-req-build-ymc5_3kd/./rpy/rinterface/r_utils.c:152: undefined reference to `Rf_lang4'

Full logs can be downloaded at:

https://github.com/mkoeppe/sage/runs/572730287

The sage-local artifact that has been built can be downloaded at

https://github.com/mkoeppe/sage/suites/584124994/artifacts/4126694

Previous tickets: https://trac.sagemath.org/search?q=rpy2+cygwin&noquickjump=1&branch=on&milestone=on&ticket=on&wiki=on

CC: @EmmanuelCharpentier @videlec @tscrim @jpflori @embray @vbraun @dimpase @jhpalmieri @darijgr @kiwifb

Component: porting: Cygwin

Author: Matthias Koeppe

Branch/Commit: 032eb86

Reviewer: Dima Pasechnik

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

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -19,3 +19,5 @@
 - https://github.com/mkoeppe/sage/suites/584124994/artifacts/4126694

+Previous tickets: https://trac.sagemath.org/search?q=rpy2+cygwin&noquickjump=1&branch=on&milestone=on&ticket=on&wiki=on
+

videlec commented 4 years ago

comment:3

Did you try the updated rpy2 package from #29441?

mkoeppe commented 4 years ago

comment:4

I haven't because this upgrade cannot be used for the 9.1 release cycle; but you can by using the testing infrastructure ticket #29403

mkoeppe commented 4 years ago

comment:6

Still a problem. I'm trying now whether an update to 2.8.6 (last py2-compatible release) helps

mkoeppe commented 4 years ago

comment:7

Same with 2.8.6

mkoeppe commented 4 years ago

comment:8

Trying #29441 now

mkoeppe commented 4 years ago

comment:9

OK, rpy2 3.2.7 from #29441 installs OK on Cygwin.

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,7 @@
-This is using system R from cygwin, build running on the [GitHub](../wiki/GitHub) Actions workflow from #29403:
+As proposed in https://groups.google.com/d/msg/sage-devel/IX3niEdmCEM/LovmF6gmIwAJ, we remove `r` from the list of standard packages. We make both `r` and `rpy2` optional packages.  This has minimal impact for users of these packages.
+
+This works around an unresolved build failure of `rpy2` on cygwin when using system R.
+This can be seen on the [GitHub](../wiki/GitHub) Actions workflow from #29403 but has also been reproduced locally.

gcc -shared -Wl,--enable-auto-image-base -L/cygdrive/d/a/sage/sage/local/lib -Wl,-rpath,/cygdrive/d/a/sage/sage/local/lib build/temp.cygwin-3.1.4-x86_64-3.7/./rpy/rinterface/_rinterface.o -L/usr/lib/R/lib -L/cygdrive/d/a/sage/sage/local/lib/python3.7/config -L/usr/lib -Lbuild/temp.cygwin-3.1.4-x86_64-3.7 -L/usr/lib/R/lib -lreadline -lR -lpcre -llzma -lbz2 -lz -ltirpc -lrt -ldl -lm -liconv -licuuc -licui18n -lpython3.7m -lr_utils -o build/lib.cygwin-3.1.4-x86_64-3.7/rpy2/rinterface/_rinterface.cpython-37m-x86_64-cygwin.dll -fopenmp

mkoeppe commented 4 years ago

Branch: u/mkoeppe/downgrade_r__rpy2_tooptionalbecause_rpy2_fails_to_build_on_cygwin_standard

mkoeppe commented 4 years ago

Commit: 6772c66

mkoeppe commented 4 years ago

New commits:

6772c66 Make r, rpy2 optional

mkoeppe commented 4 years ago

Author: Matthias Koeppe

jhpalmieri commented 4 years ago

comment:14

If #29441 fixes the problem, can't we just that instead? Or are you proposing this as a temporary solution until #29441 is ready?

mkoeppe commented 4 years ago

comment:15

We can't use #29441 because the new version is py3 only. When we make the switch to py3-only sage, we can reconsider.

jhpalmieri commented 4 years ago

comment:16

This leads to a policy question: does this failure on Cygwin mean that people on OS X, linux, etc., are deprived of having a Sage installation of R? (Does anyone download a Sage binary expecting to get a functioning installation of R?)

Actually, why should R be optional; shouldn't it just be rpy2?

mkoeppe commented 4 years ago

comment:17

Yes, we could just make rpy2 optional, that would be fine with me.

I don't really know if we have any users of R and of rpy2.

mkoeppe commented 4 years ago

comment:18

And unfortunately nobody responded to my message on sage-devel last month

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,7 +1,11 @@
-As proposed in https://groups.google.com/d/msg/sage-devel/IX3niEdmCEM/LovmF6gmIwAJ, we remove `r` from the list of standard packages. We make both `r` and `rpy2` optional packages.  This has minimal impact for users of these packages.
+In https://groups.google.com/d/msg/sage-devel/IX3niEdmCEM/LovmF6gmIwAJ, it was proposed to remove `r` (and thus everything that depends on it) from the list of standard packages. 

-This works around an unresolved build failure of `rpy2` on cygwin when using system R.
-This can be seen on the [GitHub](../wiki/GitHub) Actions workflow from #29403 but has also been reproduced locally.
+The build problems with `r` that prompted this proposal have been resolved. However, for the package `rpy2` there is an unresolved build failure on cygwin when using system R (`cygwin-standard`).
+This can be seen on the [GitHub](../wiki/GitHub) Actions workflow from #29403 but has also been reproduced locally. Details below.
+
+This ticket downgrades rpy2 to "optional". (If the binary distributions wish to continue to ship it, that is just a matter of configuring them so.)
+
+---

gcc -shared -Wl,--enable-auto-image-base -L/cygdrive/d/a/sage/sage/local/lib -Wl,-rpath,/cygdrive/d/a/sage/sage/local/lib build/temp.cygwin-3.1.4-x86_64-3.7/./rpy/rinterface/_rinterface.o -L/usr/lib/R/lib -L/cygdrive/d/a/sage/sage/local/lib/python3.7/config -L/usr/lib -Lbuild/temp.cygwin-3.1.4-x86_64-3.7 -L/usr/lib/R/lib -lreadline -lR -lpcre -llzma -lbz2 -lz -ltirpc -lrt -ldl -lm -liconv -licuuc -licui18n -lpython3.7m -lr_utils -o build/lib.cygwin-3.1.4-x86_64-3.7/rpy2/rinterface/_rinterface.cpython-37m-x86_64-cygwin.dll -fopenmp

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

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

95a72b6 Downgrade package rpy2 to optional

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

Changed commit from 6772c66 to 95a72b6

mkoeppe commented 4 years ago

comment:21

OK, how about this version

novoselt commented 4 years ago

comment:22

Users of SageMathCell certainly do care about having R available and Sage interface to it functional (which, I believe, is based on rpy2 now). It also seemed to me that 9.1 does not have to support Python2, am I wrong on this?

mkoeppe commented 4 years ago

comment:23

Replying to @novoselt:

Users of SageMathCell certainly do care about having R available and Sage interface to it functional (which, I believe, is based on rpy2 now).

You can still choose to install it, like any other optional package

It also seemed to me that 9.1 does not have to support Python2, am I wrong on this?

9.1 still supports Python 2 - see https://wiki.sagemath.org/ReleaseTours/sage-9.1

novoselt commented 4 years ago

comment:24

Replying to @mkoeppe:

Replying to @novoselt:

Users of SageMathCell certainly do care about having R available and Sage interface to it functional (which, I believe, is based on rpy2 now).

You can still choose to install it, like any other optional package

My point was however that users care about it. If they care about it in the context of SageMathCell, it is feasible that they care in other cases as well...

mkoeppe commented 4 years ago

comment:25

They can install it, and also nothing would stop our binary distributions from installing it, like any other optional packages

mkoeppe commented 4 years ago

comment:26

There is barely any integration of R into Sage. There's sage.interfaces.r (which uses rpy2) and then a single function in the sage.stats.r module. I don't think this makes sense to keep as a standard package

novoselt commented 4 years ago

comment:27

Replying to @mkoeppe:

There is barely any integration of R into Sage. There's sage.interfaces.r (which uses rpy2) and then a single function in the sage.stats.r module. I don't think this makes sense to keep as a standard package

Um, sage.interfaces.r IS the whole point of integration! We don't use it for other parts ourselves, but we make it available for users to use whatever is available through that interface. If it is optional, but binary distributions still include it, I am missing the point of what is standard. And for those who do use binary distributions it is not that easy to install optional packages.

I think dropping Python 2 support would be less noticeable than dropping R installation anywhere.

jhpalmieri commented 4 years ago

comment:28

We can drop support now and then reinstate once #29441 is ready (and we've made the transition to Python 3 only). Or we can keep the support now and have a broken build on Cygwin. Are those the two choices?

The ticket description is vague: if we build Sage's R with Cygwin, does everything work; that is, is it only a problem of the system R on Cygwin?

mkoeppe commented 4 years ago

comment:29

Replying to @jhpalmieri:

We can drop support now and then reinstate once #29441 is ready (and we've made the transition to Python 3 only). Or we can keep the support now and have a broken build on Cygwin. Are those the two choices?

Yes.

The ticket description is vague: if we build Sage's R with Cygwin, does everything work; that is, is it only a problem of the system R on Cygwin?

This should be checked. If it works with R from the sage spkg, then disabling use of the system package would be a 3rd option, probably the best one.

kiwifb commented 4 years ago

comment:32

Suddenly dropping R/rpy2 fells rough. However, we don't have usage data to know how rough it would be.

I'd prefer the third option if it proves workable. Otherwise is it possible to have it optional so that it doesn't build on cygwin but build elsewhere?

mkoeppe commented 4 years ago

comment:33

Replying to @mkoeppe:

The ticket description is vague: if we build Sage's R with Cygwin, does everything work; that is, is it only a problem of the system R on Cygwin?

This should be checked. If it works with R from the sage spkg, then disabling use of the system package would be a 3rd option, probably the best one.

It seems that this would work, but I have to check more

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

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

032eb86 build/pkgs/r/spkg-configure.m4: Do not use system R on cygwin

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

Changed commit from 95a72b6 to 032eb86

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,9 +1,8 @@
-In https://groups.google.com/d/msg/sage-devel/IX3niEdmCEM/LovmF6gmIwAJ, it was proposed to remove `r` (and thus everything that depends on it) from the list of standard packages. 
-
-The build problems with `r` that prompted this proposal have been resolved. However, for the package `rpy2` there is an unresolved build failure on cygwin when using system R (`cygwin-standard`).
+For package `rpy2` (versions 2.8.x) there is an unresolved build failure on cygwin when using system R (`cygwin-standard`).
 This can be seen on the [GitHub](../wiki/GitHub) Actions workflow from #29403 but has also been reproduced locally. Details below.

-This ticket downgrades rpy2 to "optional". (If the binary distributions wish to continue to ship it, that is just a matter of configuring them so.)
+This ticket disables use of system R on cygwin.  This should be reverted with the #29441 upgrade to more recent `rpy2` (python-3 only).
+

 ---

mkoeppe commented 4 years ago

comment:36

OK, new approach

novoselt commented 4 years ago

comment:37

Thank you for figuring it out!

dimpase commented 4 years ago

comment:38

OK, this works on non-Cygwin. Now to test on Cygwin...

dimpase commented 4 years ago

comment:39

Cygwin tests on https://github.com/dimpase/sage/actions/runs/83451150

dimpase commented 4 years ago

Reviewer: Dima Pasechnik

dimpase commented 4 years ago

comment:41

OK, this works

jhpalmieri commented 4 years ago

comment:42

Great, I'm very happy with this alternate approach!

mkoeppe commented 4 years ago

comment:43

Thanks!

vbraun commented 4 years ago

Changed branch from u/mkoeppe/downgrade_r__rpy2_tooptionalbecause_rpy2_fails_to_build_on_cygwin_standard to 032eb86

sagemath / sage

cygwin: Do not use system R #29486