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

cygwin: Do not use system R #29486

Closed
mkoeppe opened this issue Apr 9, 2020 · 43 comments
Closed

cygwin: Do not use system R #29486

mkoeppe opened this issue Apr 9, 2020 · 43 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 9, 2020

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:

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

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 mkoeppe added this to the sage-9.1 milestone Apr 9, 2020
@mkoeppe

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Apr 10, 2020

comment:3

Did you try the updated rpy2 package from #29441?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 10, 2020

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

mkoeppe commented Apr 19, 2020

comment:6

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2020

comment:7

Same with 2.8.6

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2020

comment:8

Trying #29441 now

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2020

comment:9

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

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title cygwin-standard: rpy2 fails to build Downgrade R, rpy2 to "optional" because rpy2 fails to build on cygwin-standard Apr 19, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2020

Commit: 6772c66

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2020

New commits:

6772c66Make r, rpy2 optional

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2020

Author: Matthias Koeppe

@jhpalmieri
Copy link
Member

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

mkoeppe commented Apr 20, 2020

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
Copy link
Member

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

mkoeppe commented Apr 20, 2020

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

mkoeppe commented Apr 20, 2020

comment:18

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

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Downgrade R, rpy2 to "optional" because rpy2 fails to build on cygwin-standard Downgrade rpy2 to "optional" because it fails to build on cygwin-standard Apr 20, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2020

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

95a72b6Downgrade package rpy2 to optional

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2020

Changed commit from 6772c66 to 95a72b6

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 20, 2020

comment:21

OK, how about this version

@novoselt
Copy link
Member

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

mkoeppe commented Apr 20, 2020

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
Copy link
Member

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

mkoeppe commented Apr 20, 2020

comment:25

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 20, 2020

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
Copy link
Member

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
Copy link
Member

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

mkoeppe commented Apr 20, 2020

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
Copy link
Member

kiwifb commented Apr 20, 2020

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

mkoeppe commented Apr 20, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2020

Changed commit from 95a72b6 to 032eb86

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Downgrade rpy2 to "optional" because it fails to build on cygwin-standard cygwin: Do not use system R Apr 20, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 20, 2020

comment:36

OK, new approach

@novoselt
Copy link
Member

comment:37

Thank you for figuring it out!

@dimpase
Copy link
Member

dimpase commented Apr 21, 2020

comment:38

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

@dimpase
Copy link
Member

dimpase commented Apr 21, 2020

comment:39

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

@dimpase
Copy link
Member

dimpase commented Apr 21, 2020

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Apr 21, 2020

comment:41

OK, this works

@jhpalmieri
Copy link
Member

comment:42

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 21, 2020

comment:43

Thanks!

@vbraun
Copy link
Member

vbraun commented Apr 23, 2020

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

7 participants