ipetkov / crane

A Nix library for building cargo projects. Never build twice thanks to incremental artifact caching.
https://crane.dev
MIT License
969 stars 92 forks source link

the quick-start-simple template will fail if given any practical build hook #592

Closed lolbinarycat closed 7 months ago

lolbinarycat commented 7 months ago

i started a project with this template and decided to use a phase hook to embed version information in my program.

this failed immediately as crane tried to run my postPatch hook on all of my dependancies.

this behavior should probably at least be better documented.

deifactor commented 7 months ago

It seems like in general, when using buildPackage, any provided hooks are also run during the deps builder. Which is almost definitely not correct and also means that changing the hooks causes a dependency rebuild. IMO it makes more sense to not pass hooks down to the the buildDepsOnly invocation; if users want that, they can explicitly invoke it.

ipetkov commented 7 months ago

Thanks for the feedback!

One thing that's difficult about omitting hooks/phases is that there is no single way to do it (you can set hook actions directly, or you can influence them by setting phase variables, or you can set them when the build starts, or you can take a dependency which introduces its own build hooks, and so on) so it would be a bit of a losing battle to support every possible option.

It's also worth noting that there are potential valid use cases for running build hooks in the dependency-only build (e.g. setting/updating things like .cargo/config.toml) which I don't want to preclude.


I'm still undecided on whether buildPackage's default behavior of providing cargoArtifacts if not set is something we should keep around or replace with an explicit requirement to define cargoArtifacts. I think most non-trivial projects would end up doing that anyway, so in the mean time I've opened https://github.com/ipetkov/crane/pull/605 so that hopefully this is a bit less of a stumbling block!

deifactor commented 7 months ago

Maybe I'm missing something (I don't have my NixOS machine handy so I can't check), but I'm not sure how defining attributes in commonArgs vs. in the buildPackage invocation will result in different behavior; the resulting attrset will be the same either way, right?

ipetkov commented 7 months ago

@deifactor the example appends to commonArgs, meaning adding any hooks in that attrset (not commonArgs) will only rebuild the package/workspace sources, and will not rebuild the dependency derivation (since that one will only see the unchanged commonArgs definition)

deifactor commented 7 months ago

I'm still confused. Here's my flake.nix:

{
  description = "Gamma management daemon for Wayland";

  inputs = {
    crane = {
      url = "github:ipetkov/crane";
      inputs.nixpkgs.follows = "nixpkgs";
    };
    fenix = {
      url = "github:nix-community/fenix";
      inputs.nixpkgs.follows = "nixpkgs";
    };
    flake-utils.url = "github:numtide/flake-utils";
    nixpkgs.url = "nixpkgs/nixos-unstable";
  };

  outputs = { self, fenix, crane, flake-utils, nixpkgs }:
    {
      nixosModules.default = import ./module.nix self;
    } // flake-utils.lib.eachDefaultSystem (system:
      let
        pkgs = nixpkgs.legacyPackages.${system};
        fe = fenix.packages.${system};
        toolchain = fe.combine [
          # need nightly toolchain for unstable features
          fe.latest.rustfmt
          fe.stable.toolchain
        ];
        craneLib = crane.lib.${system}.overrideToolchain toolchain;
        commonArgs = {
          pname = "cosmos";
          # don't need to build the xtask binary since the artifacts are checked in
          cargoExtraArgs = "-p cosmos";
          src = pkgs.lib.cleanSourceWith {
            src = craneLib.path ./.;
            filter = path: type:
              craneLib.filterCargoSources path type
              # need the README.md since we use that for some docstrings
              || builtins.match ".*md$" path != null;
          };
          buildInputs = [ pkgs.openssl ];
          nativeBuildInputs = [ pkgs.installShellFiles pkgs.pkg-config ];
        };

        cosmos = craneLib.buildPackage (commonArgs //
          # We directly include the doc/completion files here so that we don't
          # have to pass them through the src.
          {
            postInstall = "installManPage ${./doc/cosmos.1};"
        });
      in {
        checks = { inherit cosmos; };
        packages.default = cosmos;
        apps.default = flake-utils.lib.mkApp { drv = cosmos; };
        devShells.default = craneLib.devShell {
          checks = self.checks.${system};
          packages = with pkgs; [
            cargo-expand
            cargo-flamegraph
            cargo-outdated
            cargo-nextest
          ];
        };
      });
}

If I change the postInstall value, I still rebuild cosmos-deps-0.1.0.

ipetkov commented 7 months ago

Ah my bad, there is supposed to be cargoArtifacts = craneLib.buildDepsOnly commonArgs; in buildPackage, let me get that fixed

deifactor commented 7 months ago

Ah! Okay, now everything makes sense, thank you :)