-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Docker-related cross-compilation fixes #68900
Conversation
@@ -183,7 +196,7 @@ rec { | |||
''; | |||
|
|||
preFixup = '' | |||
find $out -type f -exec remove-references-to -t ${go} -t ${stdenv.cc.cc} '{}' + | |||
find $out -type f -exec remove-references-to -t ${go.nativeDrv or go} -t ${stdenv.cc.cc} '{}' + |
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.
In the case of cross-compilation, go
refers to the host derivation, but we need the build derivation. .nativeDrv
is only present when we're cross-compiling (this is the result of an optimization in splice.nix
).
There are many similar cases in nixpkgs where a native dependency is meant to be interpolated into a string. For example, grep for remove-references-to -t ${go}
. As in the case above, this does not work when cross-compiling.
There are no explicit references to .nativeDrv
like this in nixpkgs. Such explicit references don't feel like the right solution here, especially considering the fact that the .nativeDrv
attribute is unstable, and will someday be renamed to pkgBuildHost
. Should we add library functions to wrap explicit access?
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.
Related: #49526 (comment) and #68895
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 think buildPackages.go
would be better here? /cc @Ericson2314
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.
Especially for removing references, the go
interpolated into that string must match the one used as a nativeBuildInput
. So, buildPackages.go
would have to replace each instance of go
in that file. The drawback of this approach is that go
becomes harder to override for this package.
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.
Yes that is a the drawback, but splicing is evil in using things like builtins.tryEval
and still falling over itself when the packages are too different. It's better to use buildInputs.go
, I'm afraid. If you really want, you could do goForBuild = buildPackages.go
in the override list or something and use that.
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 agree that this is the best solution in sight right now.
# Optimizations break compilation of libseccomp c bindings | ||
hardeningDisable = [ "fortify" ]; | ||
|
||
nativeBuildInputs = [ pkgconfig ]; | ||
inherit (go.nativeDrv or go) GOOS GOARCH; |
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.
Same issue with .nativeDrv
here.
Thank you for your contributions.
|
(In response to the automatic marking of this PR as stale) I'm still interested in how cross compilations issues like this one, brought about as a side effect of splicing, are best addressed. |
Sorry for letting this getting stalled. If you could rebase this. I will have a look. |
i ran into this issue today and got it building with the help of @Mic92. this is the diff i ended up with: https://gist.github.com/betaboon/b0f7d8311297fe6ae6e751783c876272 |
@betaboon can you make this a PR? |
@betaboon If you've got a patch ready, go for it! I'm also happy to finish up if you'd prefer that. However, I noticed this added line in your gist:
This line faces the same issue that I tried to address in this PR. If This isn't a big deal in itself, but while cross-compilation Nixpkgs I found a number of similar instances. In this case, this problem doesn't get in the way of the derivation's functionality, it just may result in a bloated closure. #68900 (comment) suggests a good immediate solution for cases where this sort of issue is more of a problem. |
Great! It appears that the |
Motivation for this change
Fixes cross-compilation for:
cni
tini
containerd
docker
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)Notify maintainers
cc @