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

dnsmasq: fix cross-compilation #91422

Closed
wants to merge 1 commit into from

Conversation

betaboon
Copy link
Contributor

Motivation for this change

#91418

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@delroth
Copy link
Contributor

delroth commented Jun 24, 2020

@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"
Copy link
Contributor

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.)

Copy link
Contributor Author

@betaboon betaboon Jun 26, 2020

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?

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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

@Mic92
Copy link
Member

Mic92 commented Jun 29, 2020

Should go to staging.

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 30, 2020

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 = that should be ?= while we are at it.

@Mic92
Copy link
Member

Mic92 commented Jun 30, 2020

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 = that should be ?= while we are at it.

I am ok with merging this as it is if that email hits the dnsmasq mailing list.

@betaboon
Copy link
Contributor Author

i reworked this PR to include a .patch which mirrors the patch i just send to the mailing-list.

Comment on lines +50 to +52
+SRC ?= src
+PO ?= po
+MAN ?= man
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Contributor Author

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 :/

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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
Copy link
Member

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!

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@FRidh FRidh added this to WIP in Staging via automation Jul 2, 2020
@flokli
Copy link
Contributor

flokli commented Jul 11, 2020

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.

@betaboon
Copy link
Contributor Author

somehow the verification-mail ended up in a spam-directory ...
the patch is now here:
http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2020q3/014177.html

in the meantime i contacted Simon kelly directly and he came back to me with these thoughts:

As for the patch, my understanding is that this allows those variables
to be set by setting environment variables before invoking make. The
current arrangement is to set those either by editing the makefile, or
from the make command line, like.

make COPTS=-DHAVE_DNSSEC DESTDIR=/

I'm somewhat concerned about just letting the environment override this
stuff: it's not as if there's even a controlled namespace that all the
variables sit in: It's quite conceivable that, say BUILDDIR could be set
by something completely different and then mysteriously break a build.

I checked a few source trees that happen to be in my home directory, and
neither BIND or libnettle do this.

Is there another way to fix the problem?

@flokli
Copy link
Contributor

flokli commented Jul 11, 2020

@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 makeFlags before building, and use that instead of the patch?

@Ericson2314
Copy link
Member

Ericson2314 commented Jul 11, 2020

Yes we can do

makeFlagsArray+=("PKG_CONFIG=$PKG_CONFIG")

in preBuild or something.


@betaboon If he can forward his reply to the list, I'll be happy to respond directly.

Basically the issue is while ?= in makefiles is somewhat rare, most packages have a configure script which does silently consume environment variables, so the effect is the same. I would also point out that while nobody likes environment variables, the fact is it is the standard for distros to communicate information to tons of build systems. Meson (e.g. see https://mesonbuild.com/Reference-tables.html#compiler-and-linker-flag-environment-variables) is a good example of grudgingly supporting environment variables just for this reason.

@Ericson2314
Copy link
Member

@FRidh FRidh moved this from WIP to Needs review in Staging Aug 29, 2020
@SuperSandro2000 SuperSandro2000 added the 2.status: wait-for-upstream Waiting for upstream fix (or their other action). label Nov 29, 2020
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a 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?

@Ericson2314
Copy link
Member

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 makeFlags than a patch.

@Mic92
Copy link
Member

Mic92 commented Nov 29, 2020

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 makeFlags than a patch.

agreed.

@FRidh FRidh moved this from Needs review to WIP in Staging Dec 8, 2020
@flokli
Copy link
Contributor

flokli commented Jan 2, 2021

@betaboon can you re-roll this?

@Mindavi Mindavi added the 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on label Mar 13, 2021
@Mindavi
Copy link
Contributor

Mindavi commented Mar 13, 2021

I've applied the patch manually on the latest master(ish), that works. But the idea here was to use the makeFlags instead, right?

@Ericson2314
Copy link
Member

Yes, that's right. If you want you can open another one with that if @betaboon is too busy.

@betaboon
Copy link
Contributor Author

ah shoot. sorry i totaly forgot about this and indeed are to busy atm.

@Ericson2314
Copy link
Member

no worries!

@Mindavi Mindavi mentioned this pull request Mar 14, 2021
12 tasks
@Mindavi
Copy link
Contributor

Mindavi commented Mar 14, 2021

Closing, I've made a new PR with makeFlags.

@Mindavi Mindavi closed this Mar 14, 2021
Staging automation moved this from WIP to Done Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

9 participants