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

Nix build system for ardupilot #24178

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gsubmarine
Copy link

@gsubmarine gsubmarine commented Jun 30, 2023

No description provided.

@gsubmarine gsubmarine changed the title trying to nixify rover Nix build system for ardupilot Jun 30, 2023
@bugeats
Copy link

bugeats commented Jul 3, 2023

Hey a nix build! Nice. I have some scraps of nix in my local ardupilot, but it was left unfinished.

One of the issues I ran into was that not all of the python packages have nix packages. I ended up trying to use mach-nix to leverage requirements.txt like so:

      defaultPackage = forAllSystems (system: pkgs: mach-nix.lib."${system}".mkPython {
        requirements = ''
          flake8
          future
          geocoder
          intelhex
          lxml
          MAVProxy
          pexpect
          pygame
          pymavlink
        '';
      });

@peterbarker
Copy link
Contributor

To accept this it would need to be tucked away somewhere innocuous - Tools/nix, for example, with patches and whatnot underneath that.

I'm concerned by an out-sized maintenance burden for this vs the number of people likely to use it. The encoding of paths, locations, git hashes and various other bits and pieces in here looks like serious headache.

@gsubmarine do you really want to push for this to go in? I have reservations, but @magicrub seems to be OK with the concept. I can take this to a DevCall to see what other people think about it if you'd like (not that I seriously understand the benefits this would bring to the project, mind you) - I can really only understand the drawbacks from the PR :-)

@gsubmarine
Copy link
Author

To accept this it would need to be tucked away somewhere innocuous - Tools/nix, for example, with patches and whatnot underneath that.

I'm concerned by an out-sized maintenance burden for this vs the number of people likely to use it. The encoding of paths, locations, git hashes and various other bits and pieces in here looks like serious headache.

@gsubmarine do you really want to push for this to go in? I have reservations, but @magicrub seems to be OK with the concept. I can take this to a DevCall to see what other people think about it if you'd like (not that I seriously understand the benefits this would bring to the project, mind you) - I can really only understand the drawbacks from the PR :-)

Hey @peterbarker :)

Happy to clean it up and tuck it away. Agree the patch is gross, i wish there was another way!

Here's my case for pushing forward --

  1. Nix is awesome. Getting more traction. I know a few companies that use nix and ardupilot together and would use this ardusim impl.
  2. The beauty of nix is it's deterministic. This implementation will always work because all of the inputs are fixed. So i'm less worried about the maintenance burden.

Let me know if you're ok with it and i'll add a commit to tuck it away in Tools/nix.

Thanks for support @magicrub.

python
pkgs.git
pkgs.rsync
pkgs.gcc-arm-embedded
Copy link
Contributor

Choose a reason for hiding this comment

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

That isn't our toolchain

pkgs.rsync
pkgs.gcc-arm-embedded
pkgs.gcc_multi
pkgs.wafHook
Copy link
Contributor

Choose a reason for hiding this comment

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

? Not need

bld.add_post_fun(_post_fun)

def _git_head_hash(ctx, path, short=False):
+ return "2388afcd"
Copy link
Contributor

Choose a reason for hiding this comment

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

That is wrong, fix the build environment to build correctly...

Tools/result Outdated
@@ -0,0 +1 @@
/nix/store/qciia3l1kmjq5y8mdnsxqypxgqcgk9zs-ardusim
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems files to add to gitignore

name = "ardusim";
src = builtins.fetchGit {
url = "https://github.com/gsubmarine/ardupilot.git";
rev = "45c9dad0e5f0868423c9853570039798897b1aec";
Copy link
Author

Choose a reason for hiding this comment

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

Need to point to this side branch until this fix goes in: #29121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants