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

--workspace and --package foo build dependencies with different features (cache miss) #396

Open
Radvendii opened this issue Sep 22, 2023 · 6 comments

Comments

@Radvendii
Copy link

We've been using buildDepsOnly with --workspace to cache all the dependencies in the workspace. Unfortunately, this doesn't actually work properly if feature unification takes place. At least, I think that's the problem. It's not 100% clear.

As I understand it the issue is that when building multiple packages simultaneously, cargo will unify the features of dependencies so that it doesn't have to build the dependencies more than once. But when building the packages separately, it won't re-use the version of the package with extra features turned on, it will re-compile the dependency at that point.

Workaround:

    buildDepsOnly {
      cargoBuildCommand = "cargoWorkspace build";
      cargoTestCommand = "cargoWorkspace test";
      cargoCheckCommand = "cargoWorkspace check";
      preBuild = ''
        cargoWorkspace() {
          command=$(shift)
          for packageDir in $(${pkgs.yq}/bin/tomlq -r '.workspace.members[]' Cargo.toml); do
            (
              cd $packageDir
              pwd
              cargoWithProfile $command "$@"
            )
          done
        }
      '';
    }
@dpc
Copy link
Contributor

dpc commented Sep 22, 2023

If I understand correctly.

Yes, it's how cargo work. In our CI for the project that has dozens of members, builds 9 separate binaries etc, we have basically two types of crane-based workflows:

  • whole workspace (--workspace --all-targets --locked for everything)
  • package/package group ones (--package ... --package ... for everything)

I was thinking about introducing explicit handling of this to crane, but I never got it, and it's not that much effort to handle manually, so I'm not sure if there's a point.

@ipetkov
Copy link
Owner

ipetkov commented Sep 23, 2023

As I understand it the issue is that when building multiple packages simultaneously, cargo will unify the features of dependencies so that it doesn't have to build the dependencies more than once. But when building the packages separately, it won't re-use the version of the package with extra features turned on, it will re-compile the dependency at that point.

Yes this is precisely what I've experienced and what is likely happening here. I believe cargo does content-address the results of building different crates with different features (meaning you could build them side by side and they shouldn't clobber each other and be reusable later), but it would mean that you'd have to change the deps-only derivation to build those exact same packages in a similar way that you build the final results. This should end up building the different combinations of dependencies' feature flags and make the caching work better...

Another approach is to use cargo-hakiri to let it manage the workspace (so that you end up using a single unification of all dependency features of the workspace), though I haven't used it myself and can't speak to it more than that!

@dpc
Copy link
Contributor

dpc commented Sep 23, 2023

What I do is use --workspace --all-targets all over the CI and in scripts and tests and then just cherry-pick individual binaries into their own derivations.

@ipetkov I was just going to attempt to make a buildWorkspace PR like this one. Would you accept it?

@ipetkov
Copy link
Owner

ipetkov commented Sep 23, 2023

@dpc is the difference mostly to pass in --workspace? (I'm ignoring the part on running nextest atm)

Originally the default cargo args would pass that in, but I wanted buildPackage to emulate what cargo does by default (which is assume --workspace if the root of the project is a workspace-only manifest, but if the root is a package only that will be built). IMO setting cargoExtraArgs = "--workspace"; isn't too difficult, but feel free to start a separate discussion on it

@dpc
Copy link
Contributor

dpc commented Sep 24, 2023

@ipetkov The difference between what exactly?

"--workspace + install cargo artifacts" is what I'm after.

When dealing with multi-crate project, the best workflow for CI seems to be:

  • whole workspace deps only
  • whole workspace build
  • whole workspace tests
  • whole workspace audit
  • ...

Indeed buildPackage emulates what cargo does by default, but it's actually not very good flows for CIs. It is much better to build everything once and share all the ./target overheads.

@dpc
Copy link
Contributor

dpc commented Sep 24, 2023

@ipetkov Oh, and pnameSuffix can be there then <name>-workspace-deps-only, <name>-workspace and so on.

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

No branches or pull requests

3 participants