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: sage-7.6
Component: packages: standard Keywords: r-project, days85
Cc: jpflori, jdemeyer, jhpalmieri, vbraun Merged in:
Authors: Emmanuel Charpentier, Jean-Pierre Flori Reviewers: Jean-Pierre Flori, Jeroen Demeyer, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 4c7d5eb (Commits, GitHub, GitLab) Commit: 4c7d5eb846d40287f9fb69ddff18d2e00ae9f6d9
Dependencies: Stopgaps: use clang and --with-darwinssl on OSX

Status badges

Description (last modified by dimpase)

Rationale : R (standard package) requires libcurl (and possibly the curl executable), but stopped to package it with version 3.3.0.

See discussion on sage-devel.

Tarball: https://curl.haxx.se/download/curl-7.53.1.tar.bz2

Change History (71)

comment:1 Changed 6 years ago by charpent

Plan :

  • Create a package that builds and installs curl unconditionnally. Test that.
  • Immediately after, patch it to add a detection of an usable systemwide version, and do not install if such a version is found.

comment:2 Changed 6 years ago by charpent

  • Branch set to u/charpent/package_libcurl__and_curl__as_a_standard_package

comment:3 Changed 6 years ago by charpent

  • 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 sage-devel for discussion of conditional installation.


New commits:

2884837Initial packaging of curl : unconditionnal installation.

comment:4 follow-up: Changed 6 years ago by jdemeyer

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

Author name please...

comment:5 follow-up: Changed 6 years ago by jdemeyer

I don't like this style of logging:

CC       libcurl_la-curl_gethostname.lo

I like to see the gcc command-line 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/spkg-install)

comment:6 follow-up: Changed 6 years ago by jpflori

  • 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 ; follow-up: Changed 6 years ago by charpent

Replying to jdemeyer:

I don't like this style of logging:

CC       libcurl_la-curl_gethostname.lo

I like to see the gcc command-line 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/spkg-install)

Doable. On your head be it...

comment:8 in reply to: ↑ 4 Changed 6 years ago by charpent

  • Authors set to Emmanuel Charpentier

Replying to jdemeyer:

Author name please...

Done

comment:9 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by 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.

and to apply potential patches.

??? Clarification required. What are "potential patches" ?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by jpflori

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/spkg-install#L3

and to apply potential patches.

??? Clarification required. What are "potential patches" ?

https://github.com/sagemath/sage/blob/master/build/pkgs/ecl/spkg-install#L13

comment:11 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by jdemeyer

Replying to charpent:

Doable. On your head be it...

What do you mean with this?

comment:12 in reply to: ↑ 10 Changed 6 years ago by jdemeyer

comment:13 Changed 6 years ago by jdemeyer

The package builds fine.

comment:14 in reply to: ↑ 11 ; follow-up: Changed 6 years ago by charpent

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 miles-log logs, where the relevant information doesn't stand out (common problem with, for example, LaTeX).

comment:15 Changed 6 years ago by git

  • Commit changed from 2884837b51b874cf4630fbf593df642f30733b6c to 1588558c94c80f698b40a6033155ebf6d78e77e7

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

1588558Packaging polish.

comment:16 follow-up: Changed 6 years ago by charpent

  • Status changed from needs_work to needs_review

In this branch :

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 ; follow-up: Changed 6 years ago by jdemeyer

Replying to charpent:

I just mean that I'm not very fond of miles-log 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 charpent

Replying to jdemeyer:

Replying to charpent:

I just mean that I'm not very fond of miles-log 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 breadth-first searches).

comment:19 Changed 6 years ago by jpflori

The HTTPS/openssl issue is still an issue.

comment:20 follow-up: Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues set to HTTPS

comment:21 follow-up: Changed 6 years ago by jpflori

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 charpent

Replying to jdemeyer:

As compiled, curl supports https. You can check this with curl-config --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 charpent

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 jpflori

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 jpflori

I guess you get https support because you have openssl and its headers installed.

comment:26 follow-up: Changed 6 years ago by jpflori

And openssl is not a dependency of sage ATM.

comment:27 in reply to: ↑ 26 Changed 6 years ago by charpent

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 sage-devel ?

comment:28 follow-up: Changed 6 years ago by jhpalmieri

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 jdemeyer

Replying to jhpalmieri:

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?

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 jdemeyer

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 non-existing or non-readable 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 jdemeyer

Replying to charpent:

I'd rather not wait for it to be merged in Sage before getting the damn curl in Sage

I never said that you need to depend on that ticket. If you remove the "patching" block, it will work with or without #20692.

comment:32 follow-up: Changed 6 years ago by git

  • Commit changed from 1588558c94c80f698b40a6033155ebf6d78e77e7 to 81e3341358690d10e8ec0d14e1d7e53b8b8bf16c

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

81e3341Removed patches handling.

comment:33 in reply to: ↑ 32 Changed 6 years ago by charpent

  • Status changed from needs_work to needs_review

Replying to git:

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

81e3341Removed patches handling.

Builds OK, passes its test suite.

comment:34 follow-up: Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:35 in reply to: ↑ 34 Changed 6 years ago by charpent

Replying to jdemeyer:

Care to explain ?

comment:36 Changed 6 years ago by jdemeyer

19

comment:39 in reply to: ↑ 38 Changed 6 years ago by charpent

Replying to jpflori:

Discussion at:

Thanks for the heads-up. 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 3-4 magnitudes) than some seem to think...

comment:40 Changed 6 years ago by jpflori

  • Authors changed from Emmanuel Charpentier to Emmanuel Charpentier, Jean-Pierre Flori
  • 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

@Volker: I've added a fat mode to avoid a lot of stuff curl might be tempted to use. I did not touch the SSL/TLS options. Should I?


New commits:

c02cb61Update Curl to 7.51.0.
d31c560Add a fat mode to curl.

comment:41 Changed 6 years ago by charpent

  • 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 on libcurl's development files.
  • Make xz (existing package) and pcre (proposed new package) standard packages (see #21744 and #21770 respectively).
  • patch the R install-spkg to test for the existence and correct capabilities of libcurl before starting to install ; display an explicitly Sage-centered message and fail if not found / not correct.
  • make R depend on xz and pcre

Only *after* this interim solution works, envision the much larger task of creating a new interface.

Jean-Pierre : 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 http-only 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 sage-devel ?

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 git

  • Commit changed from d31c5607d0f4cc3ebad13388fa9d6d630dd23c11 to e4e56651e3252781f3eb04af00082f7c63be816b

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

e4e5665Upgraded curl to 7.52.1

comment:43 Changed 5 years ago by charpent

After #22189, curl remains a valid solution. I took the opportunity to upgrade curl to 7.52.1

Still needs_review


New commits:

e4e5665Upgraded curl to 7.52.1

comment:44 Changed 5 years ago by charpent

  • Description modified (diff)

comment:45 Changed 5 years ago by charpent

Sage-7.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

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

comment:46 Changed 5 years ago by charpent

  • Owner changed from (none) to charpent

Merged in #20523. Acceptingthe ticket in order to mark it as invalid.

comment:47 follow-up: Changed 5 years ago by charpent

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 charpent

Replying to charpent:

I still can't mark it as invalid. Can someone with the right rights do this for me ?

After discussion of (potential) #20523 reviewers, I now think that this ticket should be kept, reviewed and validated. Pleas DON'T invalidate it.

comment:49 follow-ups: Changed 5 years ago by jpflori

@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 charpent

Replying to jpflori:

@jeroen: I think we can merge this one ut it also entails that SSL becomes a Sage dependency. Any thought?

As per #22189 (already merged), SSL IS a Sage dependency. The point is therefore moot...

comment:51 in reply to: ↑ 49 ; follow-up: Changed 5 years ago by 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...

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 ; follow-ups: Changed 5 years ago by charpent

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 weather-fighting (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 jdemeyer

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 jdemeyer

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 jpflori

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 dimpase

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 git

  • Commit changed from e4e56651e3252781f3eb04af00082f7c63be816b to 2e615c130c342dc69fc79f6dfa1fa13d3969d7a9

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

2e615c1Merge branch 'public/curl7510' of trac.sagemath.org:sage into rupd

comment:58 Changed 5 years ago by git

  • Commit changed from 2e615c130c342dc69fc79f6dfa1fa13d3969d7a9 to bf54f54e895a416a21457e48067cb198ebae28cc

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

bf54f54bumped up the version

comment:59 Changed 5 years ago by dimpase

  • Description modified (diff)
  • Milestone changed from sage-7.5 to sage-7.6
  • Status changed from needs_review to positive_review

comment:60 Changed 5 years ago by dimpase

  • Reviewers set to Jean-Pierre Flori, Jeroen Demeyer, Dima Pasechnik
  • Status changed from positive_review to needs_review

comment:61 follow-up: Changed 5 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:62 in reply to: ↑ 61 Changed 5 years ago by charpent

Replying to dimpase:

Спассибо, Димитри. Sorry for the delay : I wasn't available this morning.

comment:63 Changed 5 years ago by dimpase

  • Status changed from positive_review to needs_work
  • Stopgaps set to use clang and --with-darwinssl on OSX

I am on it.

comment:64 Changed 5 years ago by git

  • Commit changed from bf54f54e895a416a21457e48067cb198ebae28cc to 4c7d5eb846d40287f9fb69ddff18d2e00ae9f6d9

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

4c7d5ebuse clang with --with-darwinssl for newer OSX

comment:65 Changed 5 years ago by dimpase

  • Status changed from needs_work to needs_review

comment:66 follow-up: Changed 5 years ago by jpflori

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 dimpase

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/spkg-install - from where I stole this.

comment:68 Changed 5 years ago by jpflori

  • Status changed from needs_review to positive_review

Then...

comment:69 Changed 5 years ago by jpflori

  • Keywords days85 added

comment:70 Changed 5 years ago by vbraun

Followup at #22686

comment:71 Changed 5 years ago by vbraun

  • Branch changed from public/curl7510 to 4c7d5eb846d40287f9fb69ddff18d2e00ae9f6d9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.