Opened 6 years ago
Closed 5 years ago
#21767 closed defect (fixed)
Package libcurl (and curl) as a standard package
Reported by:  charpent  Owned by:  charpent 

Priority:  major  Milestone:  sage7.6 
Component:  packages: standard  Keywords:  rproject, days85 
Cc:  jpflori, jdemeyer, jhpalmieri, vbraun  Merged in:  
Authors:  Emmanuel Charpentier, JeanPierre Flori  Reviewers:  JeanPierre Flori, Jeroen Demeyer, Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  4c7d5eb (Commits, GitHub, GitLab)  Commit:  4c7d5eb846d40287f9fb69ddff18d2e00ae9f6d9 
Dependencies:  Stopgaps:  use clang and withdarwinssl on OSX 
Description (last modified by )
Rationale : R (standard package) requires libcurl (and possibly the curl executable), but stopped to package it with version 3.3.0.
See discussion on sagedevel.
Change History (71)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
 Branch set to u/charpent/package_libcurl__and_curl__as_a_standard_package
comment:3 Changed 6 years ago by
 Commit set to 2884837b51b874cf4630fbf593df642f30733b6c
 Status changed from new to needs_review
This initial packaging :
 builds with no error on top of 7.5beta0
 passes its own test suite successfully.
The resulting Sage passes testlong with no errors (not even transient, for a marvel...
Marking needs_review
. See the thread on sagedevel
for discussion of conditional installation.
New commits:
2884837  Initial packaging of curl : unconditionnal installation.

comment:4 followup: ↓ 8 Changed 6 years ago by
 Description modified (diff)
 Status changed from needs_review to needs_info
Author name please...
comment:5 followup: ↓ 7 Changed 6 years ago by
I don't like this style of logging:
CC libcurl_lacurl_gethostname.lo
I like to see the gcc
commandline to figure out what is going on.
Usually, this can be changed by passing V=1
to the make
command line (see for example build/pkgs/libfplll/spkginstall
)
comment:6 followup: ↓ 9 Changed 6 years ago by
 Status changed from needs_info to needs_work
Would be nice to have the standard pieces of code to check we are in a Sage shell and to apply potential patches.
comment:7 in reply to: ↑ 5 ; followup: ↓ 11 Changed 6 years ago by
Replying to jdemeyer:
I don't like this style of logging:
CC libcurl_lacurl_gethostname.loI like to see the
gcc
commandline to figure out what is going on.Usually, this can be changed by passing
V=1
to themake
command line (see for examplebuild/pkgs/libfplll/spkginstall
)
Doable. On your head be it...
comment:9 in reply to: ↑ 6 ; followup: ↓ 10 Changed 6 years ago by
Replying to jpflori:
Could you clarify :
Would be nice to have the standard pieces of code to check we are in a Sage shell
??? What"'s that ? A simple hint might be to test ! X$SAGE_ROOT = X
, but you seem to have something more sophisticated in head.
and to apply potential patches.
??? Clarification required. What are "potential patches" ?
comment:10 in reply to: ↑ 9 ; followup: ↓ 12 Changed 6 years ago by
Replying to charpent:
Replying to jpflori:
Could you clarify :
Would be nice to have the standard pieces of code to check we are in a Sage shell
??? What"'s that ? A simple hint might be to
test ! X$SAGE_ROOT = X
, but you seem to have something more sophisticated in head.
https://github.com/sagemath/sage/blob/master/build/pkgs/ecl/spkginstall#L3
and to apply potential patches.
??? Clarification required. What are "potential patches" ?
https://github.com/sagemath/sage/blob/master/build/pkgs/ecl/spkginstall#L13
comment:11 in reply to: ↑ 7 ; followup: ↓ 14 Changed 6 years ago by
comment:12 in reply to: ↑ 10 Changed 6 years ago by
Replying to jpflori:
https://github.com/sagemath/sage/blob/master/build/pkgs/ecl/spkginstall#L13
Please don't, see #20692 instead.
comment:13 Changed 6 years ago by
The package builds fine.
comment:14 in reply to: ↑ 11 ; followup: ↓ 17 Changed 6 years ago by
Replying to jdemeyer:
Replying to charpent:
Doable. On your head be it...
What do you mean with this?
Nothing rude, be assured ! (sorry if that sounded rude...). I just mean that I'm not very fond of mileslog logs, where the relevant information doesn't stand out (common problem with, for example, LaTeX).
comment:15 Changed 6 years ago by
 Commit changed from 2884837b51b874cf4630fbf593df642f30733b6c to 1588558c94c80f698b40a6033155ebf6d78e77e7
Branch pushed to git repo; I updated commit sha1. New commits:
1588558  Packaging polish.

comment:16 followup: ↓ 31 Changed 6 years ago by
 Status changed from needs_work to needs_review
In this branch :
 Sage shell checking
 verbose compilation
 patching (and empty patches directory), Sage Maintainer's Guide style.
This builds and passes the package's own test suite without errors ; the resulting Sage passes ptestlong with the usual transient error on simplicial_complex.py
, which passes succesfully standalone.
Re: #20692 : This ticket has been in the works for 5 months and still depends on two other tickets. I'd rather not wait for it to be merged in Sage before getting the damn curl
in Sage : R
depends on it, and updating it begins to be a matter of some (slight) urgency : more and more R
packages can no longer be installed under 2.4.2... That doesn't mean I won't revise the way to patch things when #20692 is merged.
Anyway, the point is moot for now : there are no patches...
==>needs_review
comment:17 in reply to: ↑ 14 ; followup: ↓ 18 Changed 6 years ago by
Replying to charpent:
I just mean that I'm not very fond of mileslog logs, where the relevant information doesn't stand out
At least the relevant information exists in the log.
comment:18 in reply to: ↑ 17 Changed 6 years ago by
Replying to jdemeyer:
Replying to charpent:
I just mean that I'm not very fond of mileslog logs, where the relevant information doesn't stand out
At least the relevant information exists in the log.
Indeed. I can see your point : it's mainly a grep
vs vgrep
(non)issue ; probably coming from our respective initial curricula (a surgeon (even a dental one) needs to see (literally) a lot of things at once ; details can (usually) wait. Hence my preference for breadthfirst searches).
comment:19 Changed 6 years ago by
The HTTPS/openssl issue is still an issue.
comment:20 followup: ↓ 22 Changed 6 years ago by
 Status changed from needs_review to needs_work
 Work issues set to HTTPS
comment:21 followup: ↓ 23 Changed 6 years ago by
Without patching I fear the only solution is to make curl and R optional. Groumpf.
comment:22 in reply to: ↑ 20 Changed 6 years ago by
Replying to jdemeyer:
As compiled, curl supports https. You can check this with curlconfig protocols
.
The issue raised by Volker is valid, but has little to do with the present issue (nor with R's update...).
Furthermore, I think we shouldn't try to solve locally a systemwide problem.
comment:23 in reply to: ↑ 21 Changed 6 years ago by
Replying to jpflori:
Without patching I fear the only solution is to make curl and R optional. Groumpf.
Care to explain ? I'm a dentist of very little mind, and have trouble following you.
comment:24 Changed 6 years ago by
IIRC we cannot automatically install openssl bc of license issues... Hence no https support in curl and then R does not want to build.
comment:25 Changed 6 years ago by
I guess you get https support because you have openssl and its headers installed.
comment:26 followup: ↓ 27 Changed 6 years ago by
And openssl is not a dependency of sage ATM.
comment:27 in reply to: ↑ 26 Changed 6 years ago by
Replying to jpflori:
And openssl is not a dependency of sage ATM.
Therefore, the obvious solution is to add OpenSSL as a prerequisite to Sage. (easy patch : two words in the main README.md...).
Would *that* be a problem ?
Should we open a discussion on sagedevel ?
comment:28 followup: ↓ 29 Changed 6 years ago by
Or add curl (with SSL support) as a prerequisite to Sage. How widely installed are curl
and OpenSSL
? OS X seems to include curl
and libcurl
, and also libssl
(but is that the same as OpenSSL? or is it at least good enough?). What about the various linux distributions?
comment:29 in reply to: ↑ 28 Changed 6 years ago by
Replying to jhpalmieri:
Or add curl (with SSL support) as a prerequisite to Sage. How widely installed are
curl
andOpenSSL
? OS X seems to includecurl
andlibcurl
, and alsolibssl
(but is that the same as OpenSSL? or is it at least good enough?). What about the various linux distributions?
I would guess that curl
itself is widely available, but maybe not with "development headers" that many distros package separately.
comment:30 Changed 6 years ago by
As I tried to explain 12, please remove this patch block (especially given that there are no patches):
# Patch sources. for patch in ../patches/*.patch; do [ r "$patch" ]  continue # Skip nonexisting or nonreadable patches patch p1 <"$patch" if [ $? ne 0 ]; then echo >&2 "Error applying '$patch'" exit 1 fi done
comment:31 in reply to: ↑ 16 Changed 6 years ago by
comment:32 followup: ↓ 33 Changed 6 years ago by
 Commit changed from 1588558c94c80f698b40a6033155ebf6d78e77e7 to 81e3341358690d10e8ec0d14e1d7e53b8b8bf16c
Branch pushed to git repo; I updated commit sha1. New commits:
81e3341  Removed patches handling.

comment:33 in reply to: ↑ 32 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:34 followup: ↓ 35 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:35 in reply to: ↑ 34 Changed 6 years ago by
Replying to jdemeyer:
Care to explain ?
comment:36 Changed 6 years ago by
comment:37 Changed 6 years ago by
Some old discussions that might enlighten you:
 https://groups.google.com/forum/#!searchin/sagedevel/opensslsort:relevance/sagedevel/mbGbpRz96q0/8nsKRNyLs3oJ (OpenSSL license issue)
 https://groups.google.com/forum/#!searchin/sagedevel/opensslsort:relevance/sagedevel/Jl11JxIb2E8/1NrQmjbMKIJ (why GnuTLS which is GPL is not a drop in replacement)
comment:38 followup: ↓ 39 Changed 6 years ago by
comment:39 in reply to: ↑ 38 Changed 6 years ago by
Replying to jpflori:
Discussion at:
Thanks for the headsup. It's important, and I'm afraid that the full size of the problem hasn't yet been considered : the R ecosystem is much larger (by about 34 magnitudes) than some seem to think...
comment:40 Changed 6 years ago by
 Branch changed from u/charpent/package_libcurl__and_curl__as_a_standard_package to public/curl7510
 Commit changed from 81e3341358690d10e8ec0d14e1d7e53b8b8bf16c to d31c5607d0f4cc3ebad13388fa9d6d630dd23c11
 Status changed from needs_work to needs_review
 Work issues HTTPS deleted
comment:41 Changed 6 years ago by
 Cc jpflori jdemeyer jhpalmieri vbraun added
I think that the (pseudo)legal considerations which motivated last month's saga leave us few options.
I looked a bit closer to the idea of using an rpy2
based new interface to an external R
. This is doable, but not in the short term.
I am under the impression that the current (pexpect) interface won't work reliably with a system R (especially on "exotic" systems such as cygwin and the Mac). Therefore, depending on a systemwide installation for R is out of the question in the short term. We need an interim solution.
My current plan is :
 Mark this ticket (#21767) as invalid/won't fix, forthe (pseudo)legal reasons discussed in the saga.
 Document in the REAME.md and various places in the Installation guide that Sage depends on
libcurl
and building it depends onlibcurl
's development files.  Make
xz
(existing package) andpcre
(proposed new package) standard packages (see #21744 and #21770 respectively).  patch the
R installspkg
to test for the existence and correct capabilities of libcurl before starting to install ; display an explicitly Sagecentered message and fail if not found / not correct.  make
R
depend onxz
andpcre
Only *after* this interim solution works, envision the much larger task of creating a new interface.
JeanPierre : I think that omitting https
requirement for R is a *very* bad idea : I do not know how longer R users will be able to depend on httponly repositories ; and, as I said before, R practical use is highly dependent of access to the 9000+ R packages in existence.
Advice required. Do you think useful to open an new discussion on sagedevel ?
Note 1 : the patchbot test of #21744 is marked "pending". What does that mean ?
Note 2 : the patchbot test of #21770 fails repeatedly due to thye source tarball being unavailable on Sage standard repositories. What should I do ? The (ftp) address of the original tarball is in the ticket's header.
comment:42 Changed 5 years ago by
 Commit changed from d31c5607d0f4cc3ebad13388fa9d6d630dd23c11 to e4e56651e3252781f3eb04af00082f7c63be816b
Branch pushed to git repo; I updated commit sha1. New commits:
e4e5665  Upgraded curl to 7.52.1

comment:43 Changed 5 years ago by
comment:44 Changed 5 years ago by
 Description modified (diff)
comment:45 Changed 5 years ago by
Sage7.6.beta4 + #20523 + #21767 (this patch) + #21770 passes ptestlong witht two transient errors :
 sage t long src/sage/homology/simplicial_set_morphism.py # Bad exit: 2 sage t long src/sage/homology/simplicial_complex.py # 1 doctest failed 
Both tests pass when run standalone.
This should close the R saga (for now) by allowing the current interfaces to work with R >= 3.3.x.
Still needs_review
comment:46 Changed 5 years ago by
 Owner changed from (none) to charpent
Merged in #20523. Acceptingthe ticket in order to mark it as invalid.
comment:47 followup: ↓ 48 Changed 5 years ago by
I still can't mark it as invalid. Can someone with the right rights do this for me ?
comment:48 in reply to: ↑ 47 Changed 5 years ago by
comment:49 followups: ↓ 50 ↓ 51 Changed 5 years ago by
@jeroen: I think we can merge this one ut it also entails that SSL becomes a Sage dependency. Any thought?
comment:50 in reply to: ↑ 49 Changed 5 years ago by
comment:51 in reply to: ↑ 49 ; followup: ↓ 52 Changed 5 years ago by
Replying to jpflori:
@jeroen: I think we can merge this one ut it also entails that SSL becomes a Sage dependency. Any thought?
I have mentioned before that I really would not like that...
So far, we have always been fixing packages requiring OpenSSL such that they would work without OpenSSL (with reduced functionality). I think that can and should be done for R also.
comment:52 in reply to: ↑ 51 ; followups: ↓ 53 ↓ 54 Changed 5 years ago by
Replying to jdemeyer:
Replying to jpflori:
@jeroen: I think we can merge this one ut it also entails that SSL becomes a Sage dependency. Any thought?
I have mentioned before that I really would not like that...
But somehow failed to explain the reasons of your dislike.
So far, we have always been fixing packages requiring OpenSSL such that they would work without OpenSSL (with reduced functionality).
And that's why Sage's pip, compiled on a VM without OpenSSL, is unusable. "Reduced functionality", indeed...
I think that can and should be done for R also.
That should be done upstream :
 in R (the R Core Team introduced the SSL requirement with R 3.3 after having maintained an internal library for years...) ;
 in Python (which explicitly depends on OpenSSL).
But I really think that this is weatherfighting (and a rearguard fight, BTW). You still have to convince us that OpenSSL drawbacks (what are they ?) are worth committing a large part of our limited resources to work around it.
comment:53 in reply to: ↑ 52 Changed 5 years ago by
Replying to charpent:
But somehow failed to explain the reasons of your dislike.
It's simple: an extra dependency makes it harder to build Sage. So far, we tried hard in Sage to have a minimal set of dependencies: all dependencies to build Sage are either obviously needed (compilers, make, ...) or are installed by default on most typical operating systems (Python).
comment:54 in reply to: ↑ 52 Changed 5 years ago by
Replying to charpent:
 in Python (which explicitly depends on OpenSSL).
Python does not require OpenSSL, so I don't get your point.
comment:55 Changed 5 years ago by
Anyway, sorry for reviving this here as curl can build without ssl. The issue is with R checking that curl supports ssl and I've posted a patch on the R ticket to remove that check.
@emmanuel: can you update curl to the latest upstream version? Then I'll push the positive review button.
comment:56 Changed 5 years ago by
I'm currently checking that 7.53.1 works on OSX. I'll push the update as soon as I'm done.
comment:57 Changed 5 years ago by
 Commit changed from e4e56651e3252781f3eb04af00082f7c63be816b to 2e615c130c342dc69fc79f6dfa1fa13d3969d7a9
Branch pushed to git repo; I updated commit sha1. New commits:
2e615c1  Merge branch 'public/curl7510' of trac.sagemath.org:sage into rupd

comment:58 Changed 5 years ago by
 Commit changed from 2e615c130c342dc69fc79f6dfa1fa13d3969d7a9 to bf54f54e895a416a21457e48067cb198ebae28cc
Branch pushed to git repo; I updated commit sha1. New commits:
bf54f54  bumped up the version

comment:59 Changed 5 years ago by
 Description modified (diff)
 Milestone changed from sage7.5 to sage7.6
 Status changed from needs_review to positive_review
comment:60 Changed 5 years ago by
 Reviewers set to JeanPierre Flori, Jeroen Demeyer, Dima Pasechnik
 Status changed from positive_review to needs_review
comment:61 followup: ↓ 62 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:62 in reply to: ↑ 61 Changed 5 years ago by
Replying to dimpase:
Спассибо, Димитри. Sorry for the delay : I wasn't available this morning.
comment:63 Changed 5 years ago by
 Status changed from positive_review to needs_work
 Stopgaps set to use clang and withdarwinssl on OSX
I am on it.
comment:64 Changed 5 years ago by
 Commit changed from bf54f54e895a416a21457e48067cb198ebae28cc to 4c7d5eb846d40287f9fb69ddff18d2e00ae9f6d9
Branch pushed to git repo; I updated commit sha1. New commits:
4c7d5eb  use clang with withdarwinssl for newer OSX

comment:65 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:66 followup: ↓ 67 Changed 5 years ago by
You don't export CC so are you sure it will be taken into account?
comment:67 in reply to: ↑ 66 Changed 5 years ago by
Replying to jpflori:
You don't export CC so are you sure it will be taken into account?
it is taken into account, sure. (I checked the log, etc). By the way, the same works in r/spkginstall
 from where I stole this.
comment:69 Changed 5 years ago by
 Keywords days85 added
comment:70 Changed 5 years ago by
Followup at #22686
comment:71 Changed 5 years ago by
 Branch changed from public/curl7510 to 4c7d5eb846d40287f9fb69ddff18d2e00ae9f6d9
 Resolution set to fixed
 Status changed from positive_review to closed
Plan :