Skip to content

build git ourselves for TLS 1.2 support #159

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

Merged
merged 5 commits into from
Feb 24, 2018
Merged

Conversation

reaperhulk
Copy link
Contributor

As of today GitHub no longer allows connections < TLS 1.2. This breaks git in the manylinux1 containers since they're linked against system OpenSSL. It also breaks building the containers at all because they try to fetch the newer curl from github via the old system curl.

To resolve this we can link our own copy of git against the OpenSSL we link the Pythons against, but there are a few knock-on effects...

  1. We have to fetch a curl from a debian mirror (or switch to using Docker's ADD) to solve the bootstrap problem. To make this minimally invasive I've just swapped to the debian mirror and downgraded curl for now.
  2. We have to reorder our various package compiles to prioritize getting a functional curl ASAP.
  3. libcurl (which we need for git) can't be deleted immediately. In fact, I can't get git to build against a static libcurl (it complains about symbols that should be only in libcrypto/libssl), so for the moment I've converted it to a dynamic lib and we never delete it. That shouldn't be a big deal as the libcurl we install has a different soversion.
  4. We need expat-devel because git depends on libexpat.
  5. cmake28 uses system curl still, so that's likely to cause anybody using cmake to fetch remote resources pain in the near future.

@njsmith
Copy link
Member

njsmith commented Feb 23, 2018

fine

@reaperhulk
Copy link
Contributor Author

reaperhulk commented Feb 23, 2018

(This isn't ready for merge yet as git is not actually building properly because everything is awful)

@reaperhulk
Copy link
Contributor Author

Looks like it's all building successfully now. This should be ready for review.

Copy link

@xhochy xhochy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, this looks good (but I'm not a maintainer here)

CURL_ROOT=curl-7.57.0
# https://github.com/Homebrew/homebrew-core/blob/e3a8622111ecefe444194cade5cca3c69165e26c/Formula/curl.rb#L6
CURL_HASH=c92fe31a348eae079121b73884065e600c533493eb50f1f6cee9c48a3f454826
CURL_ROOT=curl_7.52.1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for the version downgrade is probably due to the mirror switch to Debian?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For others who might find this PR, yes this was the reason. I'd prefer to just go to 7.58.0, but that would require switching to ADD in docker. I do think we should switch to that and use less bash to build this thing now that --squash is an option, but that's best done in manylinux2.

@njsmith
Copy link
Member

njsmith commented Feb 24, 2018

Well, we're clearly trying to bail out a boat while it sinks, so I guess we better hurry up with the bailing...

(PEP 571 will be what we evacuate to.)

@njsmith
Copy link
Member

njsmith commented Feb 24, 2018

Github's just giving me a "server error" when I try to click merge...

@njsmith njsmith merged commit f7356e3 into pypa:master Feb 24, 2018
@reaperhulk reaperhulk deleted the git-fix branch February 25, 2018 02:08
@markrwilliams markrwilliams mentioned this pull request Apr 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants