-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
Conversation
@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. |
How is it different from |
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. |
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. |
Can you change the target branch of this pr from master to staging? This will rebuild quite a bit. |
This allows a policy decision about which Kerberos to use.
@@ -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; }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Ping? |
The only difference to closures I see is adding I have a WIP on the split. I just need to check reverse dependencies that they don't break after that libkrb5 change. |
/cc #29785. Otherwise we would have to put the lib in, etc.
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
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)