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

kismet: fix cross compilation #276038

Merged
merged 1 commit into from
Dec 23, 2023
Merged

kismet: fix cross compilation #276038

merged 1 commit into from
Dec 23, 2023

Conversation

pete3n
Copy link
Contributor

@pete3n pete3n commented Dec 22, 2023

Changes to allow cross-compile builds with pkgsCross
See: https://gist.github.com/pete3n/4d997ee43277d31e2d7ee2a987018f5f

Description of changes

  • Added pre-configure export of pkg-config path to prevent breaking in configure script during cross-builds
  • Added config flags for protoc and protoc-c to prevent missing dependencies during cross-builds
  • Set withPython default to false to prevent cross-build breaking issues I have not solved yet

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-linux -> aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • [x Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 22, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 22, 2023
@@ -39,6 +39,10 @@ stdenv.mkDerivation rec {
--replace "-m 4550" ""
'';

preConfigure = ''
export PATH=${pkg-config}/bin/:$PATH
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds the hostPlatform pkgs-config to PATH, which (is most cases) is not runnable on buildPlatform.

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 just want to clarify: Are you saying this breaks most builds or that this fixes most builds? Native builds are currently fine for the existing package on both aarch64 and x86_64 (probably most builds). Cross builds are currently broken (specifically x86_64 -> aarch64, but probably all cross-builds), which is what my pull request addresses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not break native build, but still won't fix cross build. If it works for you, could be binfmt_misc interfering, please disable that and try again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A proper fix is to replace all hardcoded references to pkg-config with $PKG_CONFIG, which points to the pkg-config binary (something like aarch64-unknown-linux-gnu-pkg-config) of the correct host/target platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not break native build, but still won't fix cross build. If it works for you, could be binfmt_misc interfering, please disable that and try again.

Aw, you are right. Good catch. I've been emulating pkg-config and the protoc compilers without realizing it :-|. This does not work without qemu-user-static package on Debian based machines or boot.binfmt.emulatedSystems = [ "aarch64-linux" ] on NixOS. Still, this is the only way I have been able to semi-cross-compile on Non-NixOS machines.

I guess if this isn't really suitable for a merge, I can create an issue, and then submit this gist: https://gist.github.com/pete3n/4d997ee43277d31e2d7ee2a987018f5f as a solution to the issue? I just don't want other people to struggle the way I have to build this with pkgsCross.

Copy link
Contributor Author

@pete3n pete3n Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sumitted an issue: #276165

That can be closed with either the provided git or possibly a revised version of this pull request. Obviously at your discretion. I just want to provide some workarounds to my cross-build issue.

@NickCao
Copy link
Member

NickCao commented Dec 22, 2023

Try this:

--- a/pkgs/applications/networking/sniffers/kismet/default.nix
+++ b/pkgs/applications/networking/sniffers/kismet/default.nix
@@ -1,5 +1,6 @@
 { lib
 , stdenv
+, autoreconfHook
 , binutils
 , elfutils
 , fetchurl
@@ -20,7 +21,7 @@
 , python3
 , sqlite
 , withNetworkManager ? false
-, withPython ? true
+, withPython ? stdenv.buildPlatform.canExecute stdenv.hostPlatform
 , withSensors ? false
 , zlib
 }:
@@ -37,6 +38,8 @@ stdenv.mkDerivation rec {
   postPatch = ''
     substituteInPlace Makefile.in \
       --replace "-m 4550" ""
+    substituteInPlace configure.ac \
+      --replace "pkg-config" "$PKG_CONFIG"
   '';
 
   postConfigure = ''
@@ -47,8 +50,13 @@ stdenv.mkDerivation rec {
         -i Makefile
   '';
 
+  strictDeps = true;
+
   nativeBuildInputs = [
+    autoreconfHook
     pkg-config
+    protobuf
+    protobufc
   ] ++ lib.optionals withPython [
     python3
   ];

@pete3n
Copy link
Contributor Author

pete3n commented Dec 23, 2023

Nick,
Thanks very much for this. Here are my results:

, withPython ? stdenv.buildPlatform.canExecute stdenv.hostPlatform

This seems to have inverted the previous results. Now cross-compile builds run with pkgsCross succeed but native builds with pkgs throw the Python dependency error previously seen with the cross-builds.

Builds

x86_64-linux NixOS 23.11 (no binfmt emulation)

  • pkgsCross.aarch64-multiplatform.kismet [x]
  • pkgs.kismet [] -- fails with Python dependency
  • pkgs.kismet - withPython false [x]

x86_64-linux Debian 11 (no qemu-user-static)

  • pkgsCross.aarch64-multiplatform.kismet [x]
  • pkgs.kismet [] -- fails with Python dependency
  • pkgs.kismet - withPython false [x]

aarch64-linux Ubuntu 22.04

  • pkgs.kismet [] -- fails with Python dependency
  • pkgs.kismet - withPython false [x]

@NickCao
Copy link
Member

NickCao commented Dec 23, 2023

Now cross-compile builds run with pkgsCross succeed but native builds with pkgs throw the Python dependency error previously seen with the cross-builds.

This could be the strictDeps. Try moving everything python to nativeBuildInputs

@pete3n
Copy link
Contributor Author

pete3n commented Dec 23, 2023

Now cross-compile builds run with pkgsCross succeed but native builds with pkgs throw the Python dependency error previously seen with the cross-builds.

This could be the strictDeps. Try moving everything python to nativeBuildInputs

That was it. I moved the conditional Python dependencies into the nativeBuildInputs and now all build scenarios are completing. I updated the PR with these changes. Thanks for your help. I'll be happy to see this get merged up.

@hughobrien
Copy link
Contributor

Result of nixpkgs-review pr 276038 run on x86_64-linux 1

1 package built:
  • kismet

pkg-config
protobuf
protobufc
] ++ lib.optionals withPython [
python3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
python3

This is redundant.

@NickCao
Copy link
Member

NickCao commented Dec 23, 2023

Also please squash the commits and fix the commit message, it should look like kismet: fix cross compilation

Update default.nix

kismet: fix cross compilation
@NickCao NickCao changed the title kismet: 2023-07-R1 kismet: fix cross compilation Dec 23, 2023
Copy link
Member

@NickCao NickCao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff LGTM, future works could be re-enabling python support for cross compilation. Waiting for ofborg.

@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Dec 23, 2023
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 23, 2023
@NickCao NickCao merged commit d8aba6f into NixOS:master Dec 23, 2023
23 of 25 checks passed
@pete3n pete3n deleted the update-kismet branch December 23, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants