lopsided98 / nix-ros-overlay

ROS overlay for the Nix package manager
Apache License 2.0
192 stars 77 forks source link

buildEnv: Reduce length of AMENT_PREFIX_PATH and other ROS 2 variables #342

Closed wentasah closed 7 months ago

wentasah commented 9 months ago

The purpose of ROS-specific version of buildEnv in this overlay is to reduce the length of environment variables when using many ROS packages. However, currently it works only for ROS 1 and not for ROS 2. This commit is an attempt to fix that.

Although the change looks trivial, it took me multiple full days to figure out what to change and how. The following is my understanding of how handling of environment variables works in ROS 2 and in nix-ros-overlay and why the change in this commit works. I'm not completely sure that it's all correct so feel free to complain if not.

I'm testing this change with the following flake.nix:

{
  inputs = {
    nix-ros-overlay.url = "/path/to/repo/with/this/commit";
    nixpkgs.follows = "nix-ros-overlay/nixpkgs";
  };
  outputs = { self, nixpkgs, nix-ros-overlay }:
    let
      pkgs = import nixpkgs {
        system = "x86_64-linux";
        overlays = [ nix-ros-overlay.overlays.default ];
      };
      rosDistro = pkgs.rosPackages.humble;
      buildEnv = rosDistro.buildEnv {
        paths = with rosDistro; [
          demo-nodes-cpp
        ];
      };
    in
    {
      devShells.x86_64-linux.default = pkgs.mkShell {
        name = "ros-env-test";
        packages = [ buildEnv ];
      };
      packages.x86_64-linux = {
        default = buildEnv;
        inherit (rosDistro) demo-nodes-cpp;
      };
    };
}

Running nix develop and then:

echo $AMENT_PREFIX_PATH | tr : \\n | wc -l

without this commit returns 71 entries, while with this commit the result in a single entry!

I gave this change some light testing and didn't find any problems, but if my analysis above is not completely correct, I can imagine this breaking things. I'll continue testing this in my project and I'd appreciate if others can test is too.

Thieso commented 7 months ago

This also seems to fix #362.