7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

Usual reason...

This one is a bit more complicated than usual : the Windows toolchain has changed ; however, I do not have a Windows system to test this.

Original tarballs :

Depends on #21767 Depends on #21770

CC: @tscrim @embray @sagetrac-gouezel @jpflori

Component: packages: standard

Keywords: r-project, days85

Work Issues: curl openssl

Author: Jean-Pierre Flori, Emmanuel Charpentier

Branch: f029f66

Reviewer: Emmanuel Charpentier, Dima Pasechnik

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

tscrim commented 8 years ago

comment:1

cc-ing the !Windows/Cygwin people.

embray commented 8 years ago

comment:2

What is the best process for testing a new dependency update?

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:3

I have to forfeit on this one, or at least delay for one eek minimum (RealLife overflow + ill health).

BTW, it seems that some of our current patches were created only for cygwin environment : I am unable to retrieve their necessity. Could their authors mtell me if they encountered their use in a Unix (Linux) environment ?

Sorry...

jpflori commented 8 years ago

comment:4

It seems that only the large_address_aware patch is cygwin-only, upstream report:

https://bugs.r-project.org/bugzilla/show_bug.cgi?id=15643 The other patches look general purpose.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

Description changed:

--- 
+++ 
@@ -2,10 +2,4 @@

 This one is a bit more complicated than usual : the Windows toolchain has changed ; however, I do not have a Windows system to test this.

-Plan :
-* Update to 3.3.0RC, test it on Linux.
-* Mark it as "needs review", and specifically ask for review on a Windows (cygwin ?) system.
-* Accounting for feedback about the Windows build, update to 3.3.0 (announced for May 3, 2016).
-* Put that (definitive) version up for review.
-
-The RC version is [here](https://cran.r-project.org/src/base-prerelease/R-rc_2016-04-28_r70564.tar.gz).
+The original tarball is [here](https://cran.r-project.org/src/base/R-3/R-3.3.1.tar.gz)

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:5

I've got some nonmaskable RealLife interruptions, seriously interfering with my plans. Sorry for the delay.

I updated the description to reflect the current plans.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

Branch: u/charpent/upgrade_r_to_3_3_1

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:7

The proposed patch seems valid on Linux : it passes ptestlong with errors in two doctests :

----------------------------------------------------------------------
sage -t --long --warn-long 109.1 src/sage/homology/simplicial_complex.py  # 1 doctest failed
sage -t --long --warn-long 109.1 src/sage/misc/trace.py  # 1 doctest failed
----------------------------------------------------------------------

which both pass when run standalone :

charpent@asus16-ec:/usr/local/sage-7$ sage -t --long --warn-long 109.1 src/sage/misc/trace.py
Running doctests with ID 2016-08-20-12-45-47-583c0f71.
Git branch: t/20523/upgrade_r_to_3_3_1
Using --optional=database_gap,mpir,python2,sage,sage_mode
Doctesting 1 file.
sage -t --long --warn-long 109.1 src/sage/misc/trace.py
    [9 tests, 1.79 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 1.8 seconds
    cpu time: 0.3 seconds
    cumulative wall time: 1.8 seconds
charpent@asus16-ec:/usr/local/sage-7$ sage -t --long --warn-long 109.1 src/sage/homology/simplicial_complex.py
Running doctests with ID 2016-08-20-12-46-08-790dac8d.
Git branch: t/20523/upgrade_r_to_3_3_1
Using --optional=database_gap,mpir,python2,sage,sage_mode
Doctesting 1 file.
sage -t --long --warn-long 109.1 src/sage/homology/simplicial_complex.py
    [588 tests, 6.39 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 6.5 seconds
    cpu time: 3.0 seconds
    cumulative wall time: 6.4 seconds

I have had this kind of hiccups since about 7.2 ; reports (on sage-release, mainly) did not alarm the release maintainers ; one of them has generated a tick.

Note that the large_address_aware has been lost : R 3.3.1 no longer has cygwin-specific sections in configure.ac or Makefile.in (and therefore nothing to patch) ; maybe the cygwin-relevant problems have been fixed upstream ?

I don't have a Windows machine to port Sage to ; I leave this problem to people better(?) equipped than me.

New commits:

336ee16 Upgrade R to 3.3.1 (correct on Linux).

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

Commit: 336ee16

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:8

On the other hand, I found this in the current R Installation and Administration Manual :

C.8 Cygwin

The 32-bit version has never worked well enough to pass R’s make check, and residual support from earlier experiments was removed in R 3.3.0.

The 64-bit version is completely unsupported.

Maybe we should consider to have rather an interface to system's R and make it an optional package ?

I'll broach this subject open on sage-devel (not r-devel, as I wrote incorrectly the first time...).

tscrim commented 8 years ago

comment:9

I get a failure trying to install this on Cygwin32 with

checking for lzma_version_number in -llzma... no
configure: error: "liblzma library and headers are required"
Error configuring R.

which is the same failure I had on #20190.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:10

Replying to @tscrim:

I get a failure trying to install this on Cygwin32 with
checking for lzma_version_number in -llzma... no
configure: error: "liblzma library and headers are required"
Error configuring R.
which is the same failure I had on #20190.

From the R 3.3.0 release notes :

The previously included versions of zlib, bzip2, xz and PCRE have been removed, so suitable external (usually system) versions are required (see the ‘R Installation and Administration’ manual).

So it's either :

add xz-tools (né lzma-utils) to the list of prerequisites for Sage (i. e. relying on system's liblzma) ;
adding xz-tools (at least the lzma library) to sage (cluttering it with yet one more library to maintain) ;
or relying on system's R through an interface that remains to be written...

Pick your poison...

NB : I also post this on sage-devel in the recent R thread...

embray commented 8 years ago

comment:11

On Cygwin I needed to apt-cyg install liblzma-devel (if you have apt-cyg, otherwise use setup as normal). Without the -devel package it doesn't install the import library, so -llzma doesn't work (-llzma.dll should though).

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:12

Replying to @embray:

On Cygwin I needed to apt-cyg install liblzma-devel (if you have apt-cyg, otherwise use setup as normal). Without the -devel package it doesn't install the import library, so -llzma doesn't work (-llzma.dll should though).

Okay. I just checked on my (Linux) system : xz is not installed in the $(SAGE_ROOT) tree, probably because I have it installed at system level and the build script detects that somehow. (BTW : it is an optional package.

Therefore, what is needed is : a. ensure that xz and its development files are installed, either in the system or in the Sage package) ; b. that this is done before the R installation begins ; and c. that R installation depends on xz being somehow available.

This can be simplified, at the expense of making xz a standard package, just by ensuring that xz is installed before R compilation.

I have little idea on how to proceed, since the Sage build system is still a (large) bit obscure to me. What I need is a way to check in the Makefile that xz and its development files are available, install xz if not (can an optional package be installed before the compilation of standard packages is finished ?), and make the R compilation dependent on this target.

Any ideas ?

(BTW : Cc on the sage-devel thread for advice...)

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 8 years ago

comment:13

You IMHO shouldn't change patches which haven't changed at all, that's just confusing. I wouldn't even touch patches where just the offset changed, not the context.

jpflori commented 8 years ago

comment:14

I'm working on this, especially on Cygwin as I resetuped a Cygwin install for other tickets at SD75.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:15

Please see this post on sage-devel's discussion of this topic. I need comments from people not as concerned as we are by Sage's R's future...

Jean-Pierre : did you consider repackaging (bnew style) the old pcre package ?

jpflori commented 8 years ago

comment:16

Nope I did not yet. I mostly made a clean set of patches.

jpflori commented 8 years ago

Changed branch from u/charpent/upgrade_r_to_3_3_1 to public/r311

jpflori commented 8 years ago

comment:17

I updated the set of patches. I also slightly modiifed the options passed to configure in spkg-install.

We still need to deal with the new dependencies. It seemed to me that on top of libpcre, R also depends on libcurl...

New commits:

2b7f6f4 Update patches set for R upgrade.

jpflori commented 8 years ago

Changed commit from 336ee16 to 2b7f6f4

jpflori commented 8 years ago

comment:18

libcurl is needed according to:

https://cran.r-project.org/doc/manuals/r-release/R-admin.html#Essential-and-useful-other-programs-under-a-Unix_002dalike

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 8 years ago

comment:19

The Cygwin patches to main/Makefile.in as well as nmath/standalone/Makefile.in, and etc/Makeconf.in have effects on non-Cygwin as well.

In the first two, the folder is changed unconditionally, the third may lead to overlinking on non-Cygwin.

I also don't like removing the documentation from SPKG.txt; at the very least, the patch descriptions should get copied into the patches. Renaming them IMHO isn't necessary either.

The patches have funny permissions; while you chmoded some a-x, the new ones again have u=rwx and go=rx.

In spkg-install, the lines with options to configure aren't consistently indented (in the diff at least => tabs vs. spaces used).

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 8 years ago

comment:20

Replying to @nexttime:

In spkg-install, the lines with options to configure aren't consistently indented.

... and --enable-R-shlib is redundant, as it has already been added to R_CONFIGURE a couple of lines above.

jpflori commented 8 years ago

comment:21

I'll take your comments into account or gve some justification here.

jpflori commented 8 years ago

comment:22

Replying to @nexttime:

The Cygwin patches to main/Makefile.in as well as nmath/standalone/Makefile.in, and etc/Makeconf.in have effects on non-Cygwin as well.

In the first two, the folder is changed unconditionally, the third may lead to overlinking on non-Cygwin.

Well spotted, I did not pay attention on that part.

From what I saw there was underlinking in all cases, I don't see how Cygwin or non-Cygwin can change anything but I'll double check.

Anyway I'll only apply this two only coniditonally.

I also don't like removing the documentation from SPKG.txt; at the very least, the patch descriptions should get copied into the patches. Renaming them IMHO isn't necessary either.

From what I remember there seemed to be a consensus on putting doc in patches themselves to modify a minimum set of files when adding/removing patches. Although each patch now has some info, I'll add more from what was removed from SPKG.txt.

The patches have funny permissions; while you chmoded some a-x, the new ones again have u=rwx and go=rx.

Windows funky choice, I'll fix it.

In spkg-install, the lines with options to configure aren't consistently indented (in the diff at least => tabs vs. spaces used).

I removed all tabs.

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

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

`893d3cb`	`Remove duplicated "--enable-R-shlib".`
`a79ccf1`	`Replace tabs by spaces in R spkg-install.`
`f37aedc`	`Apply some R patches only on Cygwin.`
`0b79739`	`Fix permission of R patches.`
`11d338e`	`Make description of patches more verbose.`
`c7f359e`	`Fix SAGE_BUILDING_R patch.`

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

Changed commit from 2b7f6f4 to c7f359e

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:24

On top of 6.4beta6+#21622 (now necessary to build Sage on Debian), this patch passes ptestlong with two failures :

----------------------------------------------------------------------
sage -t --long --warn-long 111.2 src/sage/homology/simplicial_complex.py  # 1 doctest failed
sage -t --long --warn-long 111.2 src/sage/libs/gap/assigned_names.py  # 1 doctest failed
----------------------------------------------------------------------

Both of them pass standalone. This kind of failure is a known, iterative (and annoying) problem, probably pertaining to the test infrastructure rather than to your patch.

I am sorely tempted to set this to positive_review, but I think that thios must be tested by someone testing it on Cygwin, given the changes in the R toolset on Windows.

Let me know if you think this is sufficient.

jpflori commented 8 years ago

comment:25

Replying to @EmmanuelCharpentier:

On top of 6.4beta6+#21622 (now necessary to build Sage on Debian), this patch passes ptestlong with two failures :
----------------------------------------------------------------------
sage -t --long --warn-long 111.2 src/sage/homology/simplicial_complex.py  # 1 doctest failed
sage -t --long --warn-long 111.2 src/sage/libs/gap/assigned_names.py  # 1 doctest failed
----------------------------------------------------------------------
Both of them pass standalone. This kind of failure is a known, iterative (and annoying) problem, probably pertaining to the test infrastructure rather than to your patch.

I am sorely tempted to set this to positive_review, but I think that thios must be tested by someone testing it on Cygwin, given the changes in the R toolset on Windows.

I tested that R with the patches shipped here buids on Cygwin, though not from a Sage env.

Let me know if you think this is sufficient.

IMHO this is largely sufficient as far as Cygwin is concerned. If by any chance there are some residual side effects on Cygwin they be fixed on follow up tickets.

More of a concern is the dependency on new packages... Can we list them here and I can try to package them:

libpcre
libcurl
anything else?

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

Changed author from Emmanuel Charpentier to Jean-Pierre Flori, Emmanuel Charpentier

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

Reviewer: Emmanuel Charpentier

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:26

Replying to @jpflori:

[ ... ]

IMHO this is largely sufficient as far as Cygwin is concerned. If by any chance there are some residual side effects on Cygwin they be fixed on follow up tickets.

Okay : positive review. BTW, I think that you are the main (first) author...

More of a concern is the dependency on new packages... Can we list them here and I can try to package them:

libpcre

libcurl

anything else?

xz-tools.

Possible strategy : 1) For now, list them as supplementary dependencies in the main README.TXT.

2) Create a set of tickets to include them in Sage. These packages shuld test for existim system functionality and, if found, do nothong (how to achieve this in a Makefile ?)

3) When those tickets are tested and positively reviewed, make them standard packages, and prerequisites for R. This pulls them off the list of prerequisites.

This way, we can buy time...

BTW : Curl and xz-tools are tools of sufficient "general" interest that we should package the whole things (i. e. binaries and docs (but maybe not test utilities ?)), not just the libraries. And add the binaries to the list of candidates for small_scripts making them accessible systemwide.

vbraun commented 8 years ago

comment:27

Hard dependency on lzma

checking for lzma_version_number in -llzma... no
configure: error: "liblzma library and headers are required"
Error configuring R.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

Dependencies: #21744

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:28

lzma is already available through the optional xz package. I opened #21744 to this order.

Trivial patch to follow...

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

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

2875b8b Three-keystroke patch ; depend on xz.

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

Changed commit from c7f359e to 2875b8b

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:30

20523 (present patch) + #21744 pass ptestlong successfully over both 7.4 and 7.5beta0.

==>needs_review

Note : I suspect that we may have to do likewise for pcre and curl. I shall package them, and open tickets to get them included as standard packages as the need arises.

jpflori commented 8 years ago

comment:31

We need to clarify the curl and openssl issues.

jpflori commented 8 years ago

Work Issues: curl openssl

jpflori commented 8 years ago

comment:32

The good news is that R compiles against an ssl-free libcurl when configure is hacked. And it seems that nothing that fails in the test suite is related to this lacking feature.

Not sure how it affects usability though.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 8 years ago

comment:33

Replying to @jpflori:

Not sure how it affects usability though.

Hah ! Most R repositories are switching to https:... addresses (this shown where trying to install a package without having defined a repository first : the graphical interface propose firs a list of repositories with https: addresses ; the only way to reach an http: repository is to click on the last item in that menu, which gives another list of http: repositories.

The move to such repositories parallels a similar move from various distros (starting with Debian...). I doubt that this will get reversed, and I think that, one way or another, Sage's R must keep the ability to use https:

This is true (with aggravation) of other uses of networking in applied statistics : access to remote databases is more and more frequent, and I do not see these uses accept insecure channels for a long time...

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 7 years ago

Description changed:

--- 
+++ 
@@ -2,4 +2,4 @@

 This one is a bit more complicated than usual : the Windows toolchain has changed ; however, I do not have a Windows system to test this.

-The original tarball is [here](https://cran.r-project.org/src/base/R-3/R-3.3.1.tar.gz)
+The original tarball is [here](https://cran.r-project.org/src/base/R-3/R-3.3.2.tar.gz)

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 7 years ago

comment:34

Changing target to 3.3.2. Meanwhile, experimenting.

Still waiting for advice (see sage-devel's sagas)...

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

Changed commit from 2875b8b to 4c896a9

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

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

`6c197f6`	`Updating R source to 3.3.2.`
`4c896a9`	`R's strig representation of source code has changed. Updated docstrings accordingly.`

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 7 years ago

comment:36

Minimal adjustments (package version + checksum) + fixed a couple of failing doctests due to R's change of representation of function source code.

These doctests might be usefully augmented by a doctest of a real R function. This doesn't seem to have ever been doctested. Advice ?

The resulting Sage (after merging with 7.5beta4) passes ptestlong with two transient failures (which pass with no errors when ran standalone).

The question of the new dependencies is still open (no answer to sage-devel's questions).

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

Changed commit from 4c896a9 to 8a05b10

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

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

8a05b10 Depend on PCRE

sagemath / sage

Upgrade R to 3.3.3 #20523

20523 (present patch) + #21744 pass ptestlong successfully over both 7.4 and 7.5beta0.