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

curl: enable Kerberos in non-fetchurl builds #29785

Merged
merged 2 commits into from Dec 19, 2017
Merged

Conversation

catern
Copy link
Contributor

@catern catern commented Sep 25, 2017

Motivation for this change

Kerberos support is needed to authenticate to some webservers and proxies, so libcurl should support it by default.

Arguably libcurl should support it even in fetchurl builds, but that grows the closure by quite a lot...

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@catern, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @vcunat and @ikervagyok to be potential reviewers.

@Mic92
Copy link
Member

Mic92 commented Sep 25, 2017

How is it different from gss?

@catern
Copy link
Contributor Author

catern commented Sep 26, 2017

Kerberos implementations are not necessarily interoperable, and the one in gss (which is GNU gss) is very rarely used. Better to use the "kerberos" package, which (theoretically) can be overridden with your desired Kerberos library (including, say, gss) so that all packages will be using that library.

@catern
Copy link
Contributor Author

catern commented Sep 26, 2017

Also, the kerberos package currently defaults to MIT Kerberos, which is much more widely used, so will make the Kerberos support in our curl much more usable.

@Mic92
Copy link
Member

Mic92 commented Sep 26, 2017

Can you change the target branch of this pr from master to staging? This will rebuild quite a bit.

@catern catern requested a review from edolstra as a code owner October 8, 2017 20:54
@catern catern changed the base branch from master to staging October 8, 2017 20:55
@@ -179,7 +179,8 @@ with pkgs;

# `fetchurl' downloads a file from the network.
fetchurl = import ../build-support/fetchurl {
inherit curl stdenv;
inherit stdenv;
curl = curl.override { gssSupport = false; };
Copy link
Member

@Mic92 Mic92 Oct 15, 2017

Choose a reason for hiding this comment

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

I would not make a different version for just fetchurl. It is pretty likely that we end up with two different libcurl versions anyway since curl is one of the most used library. So either we enable it for curl by default or have curlFull as alternative.

Copy link
Contributor Author

@catern catern Oct 16, 2017

Choose a reason for hiding this comment

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

I'm a little worried about putting it in "curl" because it grows the Nix closure. If I make a different version for just "fetchurl", I guess I don't have that problem? I don't want to put it in "curlFull", though, because curlFull isn't used when when derivations want to compile against libcurl.

What I think we should probably be doing is have a "curlBasic" version which is used by fetchurl and other things in the closure of Nix. And then make normal "curl" be "curlFull". Either we should do that, or do this.

Copy link
Member

@Mic92 Mic92 Oct 16, 2017

Choose a reason for hiding this comment

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

It will blow the closure in general. Curl is used anywhere. By having two version of libcurl it will only become worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other issue (kind of the same issue) is that if we add Kerberos support to the curl used in fetchurl, those Kerberos dependencies need to use fetchurlboot when building or there will be a circular dependency. Right?

I can do that (change them to use fetchurlboot) if that is desired.

Copy link
Member

Choose a reason for hiding this comment

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

I would like to know the opinion of the other maintainers here.

@catern
Copy link
Contributor Author

catern commented Dec 11, 2017

Ping?

@vcunat
Copy link
Member

vcunat commented Dec 17, 2017

The only difference to closures I see is adding libkrb5.out, currently ~3 MiB. If we separate include/ -> $dev, it will only be adding ~2.2 MiB. I think that would be OK, even for fetchurl (I solved the cycles). For comparison, Debian and Fedora do have a dependency from curl to kerberos.

I have a WIP on the split. I just need to check reverse dependencies that they don't break after that libkrb5 change.

@vcunat vcunat merged commit 68432fd into NixOS:staging Dec 19, 2017
vcunat added a commit that referenced this pull request Dec 19, 2017
vcunat added a commit that referenced this pull request Dec 25, 2017
vcunat added a commit that referenced this pull request Dec 25, 2017
/cc #29785.  Otherwise we would have to put the lib in, etc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants