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

Upgrade git to 2.11.0 #22058

Closed
EmmanuelCharpentier mannequin opened this issue Dec 14, 2016 · 28 comments
Closed

Upgrade git to 2.11.0 #22058

EmmanuelCharpentier mannequin opened this issue Dec 14, 2016 · 28 comments

Comments

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Dec 14, 2016

Motivation : recent changes in OpenSSL cause current git (2.6.2) to fail to compile with recent (>=1.1) version of openssl, due to changes in macros defining library function declarations.

This seems to have affected a number of packages. An example is given here, where the solution was a fix to the affected software (R package).

Tarball: https://www.kernel.org/pub/software/scm/git/git-2.11.0.tar.gz

CC: @slel

Component: packages: standard

Author: Emmanuel Charpentier

Branch/Commit: bd5a04f

Reviewer: Jeroen Demeyer, Dima Pasechnik

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

@EmmanuelCharpentier EmmanuelCharpentier mannequin added this to the sage-7.5 milestone Dec 14, 2016
@dimpase
Copy link
Member

dimpase commented Dec 15, 2016

comment:1

IHMO it's OK to go for 2.11.0.

@EmmanuelCharpentier

This comment has been minimized.

@EmmanuelCharpentier EmmanuelCharpentier mannequin changed the title Upgrade git to 2.10.2 (or 2.11 ?) Upgrade git to 2.11 Dec 16, 2016
@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Dec 16, 2016

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Dec 16, 2016

Author: Emmanuel Charpentier

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Dec 16, 2016

comment:4

The proposed patch :

  • build and installs cleanly a functional git 2.11.0
  • sort-of passes its own test suite (fails on items marked as "known breakage" in the log) :
fixed   0
success 13952
failed  0
broken  192
total   14340
----------------------------------------------------------------------
sage -t --long --warn-long 88.0 src/sage/rings/polynomial/multi_polynomial_ideal.py  # 1 doctest failed
----------------------------------------------------------------------

==> needs_review


New commits:

5f3c5f1Update source to 2.11.0, fix checksum.
4fc4c5eThe config file has to be generated.

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Dec 16, 2016

Commit: 4fc4c5e

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Dec 17, 2016

comment:5

I do not understand the urge for a "recent (>=1.1) version of openssl" (note for example that 1.1.0 entered Debian unstable less than 2 month ago, and testing only half a month ago). We are using 1.0.2, which is the current Long Term Support, and will be maintained until 2019-12 (3 years from now). Sticking to it allowed us not to be concerned by the last (2016-11-10) security fixes (which were about 1.1.0 only). It is usually easier to wait for some maturity, especially since we do not need latest openssl features (nor bugs). I am not saying that we have to stick to 1.0.2 until 2019-12, but i do not see why se should deal with the teething problems for free and no benefit (our exprience will not help R or git with that either).

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Dec 17, 2016

comment:6

You are indeed right : we (as Sage users) do not need "recent" versions of anything.

But we (as users of other software, corresponding with other users of other software) need reasonably current versions of software more or less in sync with what our "real world" correspondents use. This, of course varies from use to use. For example, to work with other statisticians using R, I need in practice to be able to use R latest released versions (R-help won't answer questions related to bugs in older versions), and in order to submit a new or revised package, I have to check it against the latest (daily !) development version.

We might also need new (real or virtual) machines (e. g. for testing purposes. So we need to accept installation on reasonably current machines. Therefore we have to be able to install Sage on these machines.

I agree that nobody in is right mind will install (and work on) debian unstable. But, except for servers purpose, nobody in his right mind will install debian stable either, at least on a personal work machine. Debian testing (or analogues in other distributions) seems to be the default choice, debian stable + backports (or reasonable facsimiles) being a distant second choice. For example, you can probably bet that Ubuntu 17.04 will have openssl 1.1.

Furthermore, this (understandable) reluctance to update can easily become a hindrance (think python2 vs python3). We have already been trapped by this in the past... and another trap is going to close on us : see this thread : it's a nice one... (not yet a ticket, because I'm not sure of the kind of answer to give).

So, all of this boils down to the definition of what is "reasonably current"... If "debian testing" or analogues are too new to be stated, the README.md file should state that.

Feel free to request a discussion on this topic on, e. g. sage-devel ; this might need clarification.

In the meantime, what do you think of the proposed patch ?

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Dec 17, 2016

comment:7

Replying to @EmmanuelCharpentier:

You are indeed right : we (as Sage users) do not need "recent" versions of anything.

Well, for example there have been various updates to latest unreleased PARI master branch, because it was useful.

But we (as users of other software, corresponding with other users of other software) need reasonably current versions of software more or less in sync with what our "real world" correspondents use.

The Long Term Support version should be one of the "reasonably current versions".

This, of course varies from use to use. For example, to work with other statisticians using R, I need in practice to be able to use R latest released versions (R-help won't answer questions related to bugs in older versions), and in order to submit a new or revised package, I have to check it against the latest (daily !) development version.

I have an apparently working way to use a recent (external) version of R in Sage/rpy2 (i did that for SDL), i will open a ticket, but i first have to close some branches because my laptop is old and i can not afford to swith branch and let it burn for recompilations.

We might also need new (real or virtual) machines (e. g. for testing purposes. So we need to accept installation on reasonably current machines. Therefore we have to be able to install Sage on these machines.

I agree that nobody in is right mind will install (and work on) debian unstable. But, except for servers purpose, nobody in his right mind will install debian stable either, at least on a personal work machine. Debian testing (or analogues in other distributions) seems to be the default choice, debian stable + backports (or reasonable facsimiles) being a distant second choice. For example, you can probably bet that Ubuntu 17.04 will have openssl 1.1.

My guess is that Debian moved 1.1.0 into testing because "The freeze is coming" (very soon), and of course Ubuntu 17.04 will follow since it is basically a snapshot of unstable (at least it was, i did not follow their recent evoltions).

Note however that Debian struggled with 1.1.0 in experimental for more that 6 months now, an that the current (in preparation) Ubuntu 17.04 still uses 1.0.2:

https://packages.qa.debian.org/o/openssl/news.rss20.xml
http://packages.ubuntu.com/zesty/openssl

I just do not understand why we have to enter such a struggle. My guess is that openssl recently changed various things, so that various softwares went broken, and it is prehaps good to just wait that the situation get stablilized, i am not claiming we should stick to 1.0.2.

Furthermore, this (understandable) reluctance to update can easily become a hindrance (think python2 vs python3). We have already been trapped by this in the past... and another trap is going to close on us : see this thread : it's a nice one... (not yet a ticket, because I'm not sure of the kind of answer to give).

This is not a reluctance. Regarding the comparison with Python 2to3, we had to wait that all our dependencies get python3 ready anyway.

So, all of this boils down to the definition of what is "reasonably current"... If "debian testing" or analogues are too new to be stated, the README.md file should state that.

The current testing is a pre-frozen, where maintainers are in a hurry to let things enter earlier because it will soon be forbidden.

Feel free to request a discussion on this topic on, e. g. sage-devel ; this might need clarification.

In the meantime, what do you think of the proposed patch ?

It looks good. Unfortunately, i currently do not have the ressource to test it seriously :/

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Dec 17, 2016

comment:8

A few points...

Replying to @sagetrac-tmonteil:

[ Snip... ]

The Long Term Support version should be one of the "reasonably current versions".

You mean Ubuntu LTS ? A bit too old on the "desktop software" and "webware" side, to the taste of my correspondents...

[ Re-Snip... ]

I have an apparently working way to use a recent (external) version of R in Sage/rpy2 (i did that for SDL), i will open a ticket, but i first have to close some branches because my laptop is old and i can not afford to swith branch and let it burn for recompilations.

This is of great interest to me : maintaining an usable R in Sage is one of my goals. However, R is a standard package. Unless a decision is made to expel it, we are bound to maintain it. And cope with its new requirements (xz, https-enabled curl, pcre).

Furthermore, keeping all the functionalities of the current pexpect interface won't be easy. This interface is highly inefficient (as underscored by William Stein, its author...), and ill-documented, but has extremely wide possibilities. Could you Cc me on this ?

[ Re-re-Snip .. ]

I just do not understand why we have to enter such a struggle. My guess is that openssl recently changed various things, so that various softwares went broken, and it is prehaps good to just wait that the situation get stablilized, i am not claiming we should stick to 1.0.2.

Whatever the motivations might be, and unless we advertise a "recent installations need nod apply" policy in the README.md file, there will be installation on (new) machines with 1.1.0 installed. Since we can't ship openssl ourselves, we have to support that.

[ Re-re-re-Snip... ]

The current testing is a pre-frozen, where maintainers are in a hurry to let things enter earlier because it will soon be forbidden.

Whatever the (good or bad) reasons, it's a fact of life we have to cope with. Don't fight the weather, man : it's an ungratifying business...

In the meantime, what do you think of the proposed patch ?

It looks good. Unfortunately, i currently do not have the ressource to test it seriously :/

Any takers ?

@jdemeyer
Copy link

jdemeyer commented Jan 2, 2017

comment:9

The tarball taken from github is NOT a proper source distribution. There should be no need to run $MAKE configure.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Jan 2, 2017

Reviewer: Jeroen Demeyer

@jdemeyer jdemeyer changed the title Upgrade git to 2.11 Upgrade git to 2.11.0 Jan 2, 2017
@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Jan 2, 2017

comment:10

Replying to @jdemeyer:

The tarball taken from github is NOT a proper source distribution.

I do not understand : this is the address pointed to by the git repository. Would you care to explain ?

There should be no need to run $MAKE configure.

Why ? Again, explanations are welcome.

@jdemeyer
Copy link

jdemeyer commented Jan 2, 2017

comment:11

What I mean is that a git repository is not the same as a source tarball. Take Sage for example: the tarball generated by make dist is not just a packaging of the git repo.

In particular, the actual source tarball of git (see new ticket description) contains the configure script, which the git repo does not contain. So, if you use the actual source tarball of git, then you do not need to run $MAKE configure.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 3, 2017

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

e3c4835Revert "The config file has to be generated."
48efb15Update checksums for git.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 3, 2017

Changed commit from 4fc4c5e to 48efb15

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Jan 3, 2017

comment:13

installs cleanly and passes its own test suite.

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Jan 3, 2017

comment:15

Replying to @jdemeyer:

What I mean is that a git repository is not the same as a source tarball. Take Sage for example: the tarball generated by make dist is not just a packaging of the git repo.

In particular, the actual source tarball of git (see new ticket description) contains the configure script, which the git repo does not contain. So, if you use the actual source tarball of git, then you do not need to run $MAKE configure.

Thanks a lot : I just followed the github link to the archives and was not aware of the kernel.org source... For my edification : where is this documented ?

@jdemeyer
Copy link

jdemeyer commented Jan 4, 2017

comment:16

Replying to @EmmanuelCharpentier:

For my edification : where is this documented ?

Good question, it's hard to find. I found it under the "Older releases" link at https://git-scm.com/downloads

@jdemeyer
Copy link

jdemeyer commented Jan 4, 2017

comment:17

I would prefer the commits to be squashed (1 commit instead of 4). Can you do that?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 4, 2017

Changed commit from 48efb15 to bd5a04f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 4, 2017

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

bd5a04fUpdate source to 2.11.0, fix checksum.

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Jan 4, 2017

comment:19

Replying to @sagetrac-git:

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

bd5a04fUpdate source to 2.11.0, fix checksum.

1've checked that git builds and passes its own testsuite :

[git-2.11.0] fixed   0
[git-2.11.0] success 13952
[git-2.11.0] failed  0
[git-2.11.0] broken  192
[git-2.11.0] total   14340

I'd rather not make ptestlong on this machine, which is too damned slow for the test (about 4 hours...).

@EmmanuelCharpentier
Copy link
Mannequin Author

EmmanuelCharpentier mannequin commented Jan 15, 2017

comment:20

Against Sage 7.5 built on a pristine Debian (testing) VM + current system's compilers and OpenSSL + development libs (libssl-dev, currently 1.1.0c) :

  • Passes its own testsuite, with similar results :
fixed   0
success 13276
failed  0
broken  185
total   13728
  • The resulting sage passes ptestlong with no errors.

One notes that the testsuite

  • depends on an utility (msgfmt, in Debian's gettext package), that seems unneeded for anything else in Sage ;
  • that the number of tests seems to depend on other installed software.

Still needs_review.

@dimpase
Copy link
Member

dimpase commented Jan 15, 2017

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Jan 15, 2017

comment:21

looks good to me.

@vbraun
Copy link
Member

vbraun commented Jan 21, 2017

Changed branch from u/charpent/upgrade_git_to_2_10_2__or_2_11___ to bd5a04f

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

3 participants