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
dnsmasq: fix cross-compilation #91422
Conversation
@ofborg build pkgsCross.aarch64-multiplatform.dnsmasq Tested manually and it seems to build fine. |
@@ -32,6 +32,7 @@ stdenv.mkDerivation rec { | |||
"BINDIR=$(out)/bin" | |||
"MANDIR=$(out)/man" | |||
"LOCALEDIR=$(out)/share/locale" | |||
"PKG_CONFIG=${buildPackages.pkgconfig}/bin/${buildPackages.pkgconfig.targetPrefix}pkg-config" |
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.
Isn't there a $PKG_CONFIG
variable available now? (So you don't have to have all that nix code.)
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 didnt find anything of the likes.
maybe @Ericson2314 knows more?
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.
Hmm. The setup hook from pkg-config will already define the environment variable. I guess from the look of it this Makefile might have trouble picking up definitions from the environment. Maybe try patching it to use ?=
instead of =
?, and I bet we can get a lot of those makeFlags
.
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.
good idea. just did and it seems to work. pushed changes
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.
Unless the patch goes upstream I actually prefer not patching the code over defining makeflags.
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.
see e.g. #91457 for the $PKG_CONFIG variable
Should go to staging. |
I know it's a lot of hoops to jump through, but can we get a patch submitted to the mailing list? There are probably other |
I am ok with merging this as it is if that email hits the dnsmasq mailing list. |
2957379
to
65ce697
Compare
i reworked this PR to include a |
+SRC ?= src | ||
+PO ?= po | ||
+MAN ?= man |
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.
Not sure if they want this on the dnsmasq list, but I assume it will be simple enough for the maintainers there to remove these 3 lines from the patch again.
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.
yeah i thought about that to. they scoped the vars in the section Variables you might want to override
so i thought: let them decide
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.
Hmm http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/ @betaboon's message didn't seem to pop up yet. Maybe it is awaiting moderation?
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 sent it twice, tried to register twice nothing worked :/ i guess i have to contact simon kelly directly :/
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 was able to register, but no idea whether the helps with moderation or not.
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.
Yeah, I propose registering and re-sending - I assume the incoming message queue of unregistered mail adresses is not looked at:
To control spam, only subscribers are allowed to post to the list.
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 heard scary things about dnsmasq upstream. Apparently the are pihole maintainer have better means to send fixes to dnsmasq upstream.
@@ -19,6 +19,10 @@ stdenv.mkDerivation rec { | |||
sha256 = "1yzq6anwgr5rlnwydpszb51cyhp2vjq29b24ck19flbwac1sk73l"; | |||
}; | |||
|
|||
patches = [ | |||
./makefile-defaults.patch |
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.
Could you also post a link to mailing list archive if available. That makes
it easier to track the upstream status. Thanks!
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.
will do as soon as available
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.
That's still missing the link to the ML archive, or given there hasn't been too much positive reaction, we might probably just want to fall back to set this in makeFlags
.
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 let's just do make flags, but still link the mailing list.
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 might be able to take care of this in the coming days. but can't promise as I'm currently very busy.
I can confirm this at least fixes cross-compilation on armv7l and aarch64, but we should have at least sent the patch to the mailing list and added a reference to it next to the patch. |
somehow the verification-mail ended up in a spam-directory ... in the meantime i contacted Simon kelly directly and he came back to me with these thoughts:
|
@Ericson2314 might have some more real-world examples of packages using this. On the other hand, shouldn't we be able to configure this by mutationg |
Yes we can do
in @betaboon If he can forward his reply to the list, I'll be happy to respond directly. Basically the issue is while |
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.
Can you please target staging with the mass rebuild?
Based on the upstream's disapproval of the patch (they don't want to be limited to GNU Make, that's understandable), it's probably better to use |
agreed. |
@betaboon can you re-roll this? |
I've applied the patch manually on the latest master(ish), that works. But the idea here was to use the makeFlags instead, right? |
Yes, that's right. If you want you can open another one with that if @betaboon is too busy. |
ah shoot. sorry i totaly forgot about this and indeed are to busy atm. |
no worries! |
Closing, I've made a new PR with makeFlags. |
Motivation for this change
#91418
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)