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

Allow cargoArtifacts in buildDepsOnly #293

Closed
wants to merge 2 commits into from
Closed

Allow cargoArtifacts in buildDepsOnly #293

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 7, 2023

Motivation

A project I am working on is structured as a single repo with multiple sub-crates, but it cannot use cargo workspaces to build them all at once (for unrelated reasons). In particular, it has a library crate and multiple application crates that have path dependencies on that library. I would like to make sure that the library is only built once by using buildDepsOnly on the library, and then passing the resulting derivation as cargoArtifacts into each application's buildDepsOnly to reuse the target directory of the library in each application. I also added a test that illustrates what I'm trying to accomplish, and it appears to exhibit the correct reuse with this change.

Checklist

  • added tests to verify new behavior
  • added an example template or updated an existing one
  • updated docs/API.md (or general documentation) with changes
  • updated CHANGELOG.md

@ipetkov
Copy link
Owner

ipetkov commented Apr 8, 2023

Thank you for the PR @akiekintveld, I appreciate that you have included a test case as well as updated the documentation and changelog!

Although the approach here is not unreasonable, my hesitation in accepting this change is because I'm not sure if it meaningfully solves the mentioned use case: if any file in the common sub-directory changes it will not only rebuild the application crate itself but all of its (additional) dependencies too.

but it cannot use cargo workspaces to build them all at once (for unrelated reasons).

It's worth mentioning that cargo will automatically promote path dependencies as workspace members, so I'm curious to hear more about this and whether it's a technical limitation or simply wanting to control the caches better. Currently there isn't a great story for having granular caching of individual crates within a workspace, which might be what you are running into here #180

(It's also worth noting that anyone blocked on this can easily inject a cargo target directory via (buildDepsOnly args).overrideAttrs (_old: { inherit cargoArtifacts; }))

@ghost
Copy link
Author

ghost commented Apr 8, 2023

Hi! Sadly my use case isn't quite covered by #180. My project consists of a library crate and two dependent application crates, but both applications have to be built for different targets. One application is built for the host target, and the other is built for riscv64imac-unknown-none-elf. There are efforts being made in cargo to support per-crate build targets in workspaces (rust-lang/cargo#7004), but the feature is unstable and doesn't quite work as expected yet. So, I'm not using a workspace.

I'm currently using this branch because I noticed that, without it, nix ends up building the library crate when I run the library tests, and building the library crate a second time when I build/run the application. When I use cargo directly, I use a shared target directory for all three crates so I only have to build the library crate once for each target.

While the limitation you mentioned is a little unfortunate, that is what I expected. It would be nice if the other application dependencies didn't have to get re-built if just the library crate changed, but for my current use-case I think that's acceptable.

Great point on the injection trick! I didn't think of using overrideAttrs to workaround this.

@ghost
Copy link
Author

ghost commented Apr 29, 2023

Just wanted to followup and see if you'd like to merge this PR, or if you'd prefer this use case be covered by the overrideAttrs workaround?

@ipetkov
Copy link
Owner

ipetkov commented Apr 29, 2023

Apologize for not following up on this earlier! I think given that overideAttrs makes for a good escape hatch I'd prefer we close this PR and keep buildDepsOnly a bit more opinionated at this time. Hopefully we'll improve the story for larger workspaces such that this dance won't be necessary in the future...

In the mean time, feel free to bring up other thoughts or improvements you may have in mind; thanks again for taking the time to open this PR!

@ipetkov ipetkov closed this Apr 29, 2023
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

Successfully merging this pull request may close these issues.

1 participant