-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
git: Don't depend curl-config
when cross-compiling.
#61552
Conversation
Fixes building with `pkgsStatic`, where otherwise curl linker flags would not be correctly determined.
@@ -76,6 +77,9 @@ stdenv.mkDerivation { | |||
configureFlags = stdenv.lib.optionals (stdenv.buildPlatform != stdenv.hostPlatform) [ | |||
"ac_cv_fread_reads_directories=yes" | |||
"ac_cv_snprintf_returns_bogus=no" | |||
# Provide curl manually, do not rely on `curl-config` | |||
# See also `CURL_LDFLAGS` below. | |||
"--with-curl=${symlinkJoin { name = "curl-libs-and-headers"; paths = [ curl.out curl.dev ]; }}" |
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.
Actually maybe try CURL_CONFIG=${curl.dev}/bin/curl-config
? from here: https://github.com/git/git/blob/master/configure.ac#L597
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.
Using symlinkJoin is usually a bad idea
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.
Using symlinkJoin is usually a bad idea
@matthewbauer Why?
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 breaks the closure size reductions from multiple outputs. When you link to that lib directory, you will also get the headers in closures.
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.
Ah, I see. That makes sense.
What should I do though, given that git has only a single flag as opposed to one for headers and libs each?
I can try the curl-config
approach, but using explicit flags sems to have less surprises so that might be preferable.
Do you no longer have the concerns about curl-config
from #61527, or is the difference of your proposal above that we'd pass the CURL_CONFIG
manually and that would somehow be better / address your previous concern of
Otherwise you wind up linking to the nonstatic curl I think
?
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.
@matthewbauer A short ping for my questions above :)
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.
Sorry. Yeah you can use curl-config, you just can't put it in nativeBuildInputs, otherwise you get the build system's curl-config binary, not the targeted system. I believe putting "CURL_CONFIG=${curl.dev}/bin/curl-config"
in configureFlags should be okay (if a little awkward).
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.
@matthewbauer Except that curl-config won't work if you're cross-compiling to a different architecture, e.g. armv7l.
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.
@kisik21 Some packages natively compile the config tools that go in the dev
output precisely for this reason. It's ugly either way, but doing that is practical.
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/proposal-a-dedicated-cross-compiling-team/3824/2 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/proposal-a-dedicated-cross-compiling-team/3824/1 |
I tried to build it with the following: (perl support disabled because of #66741)
It got me the following:
I think that it's a bug somewhere in the symlinkJoin function. |
I wanted to look more into this today, but got a different dependency failing:
Any idea what that is? |
Thank you for your contributions.
|
@nh2 Can you please resolve the merge conflict? |
I'll have to re-test this thoroughly then and also look in detail into the feedback given above; converting back to draft PR for now. |
I marked this as stale due to inactivity. → More info |
pkgsStatic.git almost builds but fails at |
Motivation for this change
Fixes building with
pkgsStatic
, where otherwise curl linker flagswould not be correctly determined.
Better approach than in #61527.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)