Refactor flake.nix

[SchahinRohani]

Description

Refactoring the flake into tools for better accessability.

Depends on this PR #1268.

How Has This Been Tested?

The command nix flake checkand nix flake show are throwing errors, so i need to refactor the flake first to be able to test it properly.

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[adam-singer]

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 46 files reviewed, and pending CI: Docs Deployment, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), and 1 discussions need to be resolved

---

tools/images/toolchain-drake.nix line 10 at r2 (raw file):

  fromImage = pullImage {
    imageName = "localhost:5001/toolchain-drake";
    imageDigest = ""; # DO NOT COMMIT DRAKE IMAGE_DIGEST VALUE

Keep in mind that https://github.com/TraceMachina/nativelink/blob/main/tools/toolchain-drake/toolchain-drake.sh#L12 does try to patch this file durning a toolchain build, it does a patch and then reverts the change when its done. So if this files path changes it will need to be reflected here.

[aaronmondal]

TBH I'm not a fan of this. The only thing this accomplishes is to reduce a few lines in the flake.nix at the cost of much higher complexity pushed into other directories, increased maintenance overhead for the factored-out parts and a higher requirement to know about the directory layout to do meaningful changes (i.e. you now have to change a bunch of files and know where things are instead of just editing a single file).

At first it might look like a nice cleanup to have a clearer separation for the different parts, but in many places this separation is "off" or wrong. Also in its current state this regresses fairly heavily on import vs callPackage usage.

I'm aware that the current state of the flake.nix file can be overwhelming, but the most important improvement to this is not part of this PR, which would be to move nativelink into it's own default.nix file. The reason this wasn't done yet is mainly because we have fairly specific requirements for crosscompilation that doesn't fit too well with the classic layouts in nixpkgs and other flake repos.

I'd be fine with moving the images like siso-chromium and drake somewhere else, but even for images in general it becomes unclear whether nativelink-worker-init should be in images or considered a part of a nativelink package.

I'd prefer if we dropped this PR and made smaller, more gradual changes (perhaps starting with only drake and siso which are "safe" to move around) instead of refactoring this many things at once.

Reviewed 26 of 26 files at r1, 22 of 22 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and pending CI: Docs Deployment, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), and 19 discussions need to be resolved

---

.gitignore line 21 at r2 (raw file):

lre.bazelrc
rust-project.json
.flake.nix

what is this file?

---

flake.nix line 221 at r2 (raw file):

        generate-toolchains = pkgs.callPackage ./tools/toolchains/generate.nix {inherit rbe-configs-gen;};

        buck2-toolchain = import ./tools/buildsystem/buck2.nix {

Prefer the pkgs.callPackage syntax.

When you have e.g. this:

x = pkgs.callPackage ./myfile.nixx {}

You can change the myfile.nix inputs arbitrarily without modifying the line above:

{
some,
inputs,
}

# use the inputs

{
some,
new,
inputs,
}

#use the inputs, doesn't require changing the top-level callPackage file.

Also this behaves better with crosscompilation since you can globablly override/patch pkgs.callPackage to e.g. pkgs.pkgcCross.sometarget.callPackage or patched-pkgs.callPackage without changing the implementation file.

---

flake.nix line 241 at r2 (raw file):

        nativelink-image = import ./tools/images/nativelink.nix {
          inherit self;
          inherit buildImage;

In this case the above pkgs.callPackage argument still holds.

In general, when you have multiple inputs that can be inherited, use this syntax instead:

inherit
  a
  b
  c
  ;

This is more idiomatic and allows

---

flake.nix line 272 at r2 (raw file):

      in rec {
        _module.args.pkgs = let
          nixpkgs-patched = import ./tools/patches/apply.nix {

Let's keep the previous implementation here for not to make it as obvious as possible which patches are applied. I think it's a good idea to move this out somewhere, but I'll need some time to think about whether this is the optimal approach.

---

flake.nix line 367 at r2 (raw file):

        };

        # Example for another devShell called "test"

No need for this example. Let's just remove it.

---

tools/buildsystem/buck2.nix line 2 at r2 (raw file):

{
  pkgs,

Don't add pkgs to inputs. Instead, use the callPackage syntax and in this case do for instance this:

{
  buildImage,
  self,
  coreutils
  bash
  go
  diffutils
  gnutar
  gzip
  python3Full
  unzip
  zstd
  cargo-bloat
  mold-wrapped
  reindeer
  lld_16
  clang_16
}: let
  buck2-nightly-rust-version = "2024-04-28";
  buck2-nightly-rust = pkgs.rust-bin.nightly.${buck2-nightly-rust-version};
  buck2-rust = buck2-nightly-rust.default.override {extensions = ["rust-src"];};
in
  pkgs.callPackage ../nativelink/create-worker-experimental.nix {
    inherit buildImage self;
    imageName = "buck2-toolchain";
    packagesForImage = [
      coreutils
      bash
      go
      diffutils
      gnutar
      gzip
      python3Full
      unzip
      zstd
      cargo-bloat
      mold-wrapped
      reindeer
      lld_16
      clang_16
      buck2-rust
    ];
  }

I know that this looks like a lot of duplication, but it let's us do things like this:

mytoolchain = (pkgs.callPackage ./mytoolchain.nix {
  buck2-rust;
  lld = pkgs.lld_18; # Override a single input
})

Note how this now lets you override specific inputs as opposed to "all or nothing":

mytoolchain = pkgs.callPackage ./myshell.nix {
  # Only override clang.
  inherit (pkgsCross.musl64) clang;
};

# Or override everything
mytoolchain = pkgs.pkgsCross.musl64.callPackage ./mytoolchain.nix {};

# Here we have no way to selectively change tools
mytoolchain = import ./mytoolchain.nix {
  pkgs = pkgs.pkgsCross.musl64;
}

---

tools/images/nativelink.nix line 1 at r2 (raw file):

{

Let's not move this one. The way that crosscompilation interacts with this is dependent on the exec platform of buildImage. This is not ideal at the moment, but factoring it out of the main flake.nix makes it harder to fix this issue.

---

tools/images/siso-chromium.nix line 1 at r2 (raw file):

{

@adam-singer @SchahinRohani I wonder, should these live in tools/images or would a top-level images directory be more appropriate? I think I'd prefer a top-level directory which would also make it easier for other users to see where the images live, as this is a fairly common thing to look at when browsing a repo.

---

tools/images/toolchain-drake.nix line 10 at r2 (raw file):

Previously, adam-singer (Adam Singer) wrote…

Keep in mind that https://github.com/TraceMachina/nativelink/blob/main/tools/toolchain-drake/toolchain-drake.sh#L12 does try to patch this file durning a toolchain build, it does a patch and then reverts the change when its done. So if this files path changes it will need to be reflected here.

Ah I totally forgot about this. Maybe we should leave some sort of reference to the patching file here?

---

tools/patches/apply.nix line 8 at r2 (raw file):

  name = "nixpkgs-patched";
  src = self.inputs.nixpkgs;
  # To apply a patch on a pkg, go into the nixpkgs repository

nit: Use the full 80 character line length for comments and write out package.

---

tools/shell/base.nix line 2 at r2 (raw file):

{
  config,

This seems like an unnecessary addition of complexity. The idiomatic approach here would be to put every package in the inputs. Since the devShell changes frequently that would mean that we'd have to change this file in multiple places all the time.

There is also not too much to gain from using different devshells as outlined in #1262 (comment)

Let's keep this in the top-level flake to make it easy for users to add/remove things to the dev environment.

---

tools/shell/test.nix line 2 at r2 (raw file):

{pkgs, ...}:
pkgs.mkShell {

Remove

---

tools/nativelink/create-worker.nix line 1 at r2 (raw file):

{

I'm not convinced that it's necessary to add a new level of indirection for this. Keep in mind that every additional directory increases friction in terminal-based editors.

Also, this is technically a utility to create an image, so I'd probably either leave it in tools or put it in images.

---

tools/nativelink/create-worker-experimental.nix line 1 at r2 (raw file):

{

as above

---

tools/nativelink/worker-init.nix line 1 at r2 (raw file):

{

This is an image.

---

tools/patches/nixpkgs_bun.diff line 29 at r2 (raw file):

         url = "https://github.com/oven-sh/bun/releases/download/bun-v${version}/bun-darwin-x64-baseline.zip";
-        hash = "sha256-5PLk8q3di5TW8HUfo7P3xrPWLhleAiSv9jp2XeL47Kk=";
+        hash = "sha256-gaDYzANmTTsjIlTXqqCWcOT7NTscuEUINsrjEb0eTRs==";

I believe this needs to be fixed in #1268

---

tools/toolchains/customClang.nix line 1 at r2 (raw file):

{

A toolchains directory is IMO a very dangerous thing to add as we have many different kinds and forms of toolchains. The separation between them is not always clear and there is a risk of creating "too much separation" between the different things that make up a toolchain.

---

tools/toolchains/generate.nix line 2 at r2 (raw file):

{
  bazel_7,

This one should actually live in local-remote-execution.

---

tools/toolchains/llvmStdenv.nix line 1 at r2 (raw file):

{

same as with the customClang comment. Let's not move this for now because I need time to think about what the best approach is for this.