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

build process rebuilds all crates #351

Closed
brianmay opened this issue Jul 5, 2023 · 8 comments
Closed

build process rebuilds all crates #351

brianmay opened this issue Jul 5, 2023 · 8 comments

Comments

@brianmay
Copy link

brianmay commented Jul 5, 2023

As discussed here: https://hachyderm.io/@penguin_brian/110658388881357896

Log showing problem: https://gist.github.com/brianmay/350d6a1832f7d2816718d59e1a35d80c

Source code: https://github.com/brianmay/robotica-rust/blob/main/flake.nix

The recommended change did not help: brianmay/robotica-rust@31fc41b

(I also tried changing the value to cargoArtifacts but that didn't help either)

@dpc
Copy link
Contributor

dpc commented Jul 5, 2023

https://github.com/brianmay/robotica-rust/blob/31fc41bf44c46a91b7e17f2ec9fa99bee47ba392/flake.nix#L29

src = ./.;

Try at least basic filtering:

src = craneLib.cleanCargoSource (craneLib.path ./.);

@dpc
Copy link
Contributor

dpc commented Jul 5, 2023

Oh dear. There's more wrong with that setup.

What's important to understand is that most of crane's derivation produce a copy of the cargo's ./target/ directory a a result. So when you do cargoArtifacts = clippy; it means roughly "restore the result of "clippy" derivation into ./target so this build step (derivation) can reuse whatever was built there. So when producing crane-based CI pipeline you need think (and possibly experiment) which steps benefit from previous work of other steps.

So generally you want to structure derivations as:

  • deps only (starts from scratch)
  • build whole workspace (starts with what "deps only" built)
  • tests (starts with "build whole workspace")
  • coverage (starts with "build whole workspace")

BTW. @ipetkov It seems to me that oftentimes running clippy, tests, coverage in separate steps is jus produces tons of artifacts for no good reason. I was thinking if it wouldn't be better to have:

  • deps only
  • "multistep" derivation, where the user just enables things they want with clippy = true;, docs = true;, tarpaulin = true; etc. so that they all run in one derivation. For many users they all either pass, or the whole build should fail anyway and nothing will get reused from it anyway, as changes will be required to something to fix it, which will invalidate all these steps anyway.

@brianmay
Copy link
Author

brianmay commented Jul 5, 2023

I copied this from an example somewhere. But I cannot find it anymore. Guess I will have to redo it.

What is a good example I should use?

@ipetkov
Copy link
Owner

ipetkov commented Jul 5, 2023

@brianmay I think I see the issue! Your root Cargo.toml specifies a package entry meaning that running cargo build without any flags is basically the same thing as running cargo build -p robotica-rust, a package which has no dependencies.

When you run cargo build -p robotica-slint cargo will try to build the robotica-slint package and all of its dependencies, except the specified cargoArtifacts only built robotica-rust's (non-existent) dependencies, so the derivation ends up doing everything from scratch...

There's a couple of different ways to fix this, all of which depend on how you'd like to build the workspace:

  • set cargoExtraArgs = "--workspace"; in all the different sub derivations which will build the entire workspace (though do be careful about building everything first and then building a single -p whatever afterwards as feature unification for one crate vs the entire workspace can result in some cache invalidation)
  • Alternatively you can get rid of the (placeholder?) robotica-rust package altogether which will result in cargo build applying to the entire workspace by default
  • Alternatively if you only care about building the robotica-slint package you can specify cargoExtraArgs = "-p robotica-slint"; everywhere

I hope this clarifies things somewhat but I'm happy to elaborate if they don't. I'll also update the docs to highlight this kind of pitfall!


@dpc it's entirely possible to set a compound build command (e.g. cargo clippy; cargo tarpaulin; cargo doc) which will only produce one total amount of artifacts. The current helpers are there to make it easier to glue stuff together but they aren't strictly required. I was hoping to make more progress on #76 but I lost a bit of steam with backwards compat wrangling

@brianmay
Copy link
Author

brianmay commented Jul 6, 2023

Hmmm. This makes sense.

robotica-rust is the top level package. Unless I have misunderstood something, I think the top level package is required to enable the sub packages.

For now, I guess the 3rd option probably should work. (except nothing is that simple, https://github.com/brianmay/robotica-rust/blob/fix_flake/flake.nix gives me other, possibly unrelated errors https://gist.github.com/brianmay/74681e5506d51153113fb4d4be53409c - should I open another bug report for this?)

But eventually I will want to build the binaries associated with the brian-backend package also. Which in turn requires building brian-frontend using wasm-pack (see Dockerfile for current build steps). Trying to work this out is going to be fun. Which is why I started off with just robotica-slint.

Wondering if I should be splitting some of this stuff into its own repo and/or workspace. Maybe at least for the wasm stuff. That way I don't have to worry wasm dependencies getting built for non-wasm builds.

@brianmay
Copy link
Author

brianmay commented Jul 6, 2023

I reproduced - what appears to be the other error - with a simpler project, and opened another bug report: #353

@brianmay
Copy link
Author

brianmay commented Jul 6, 2023

Note to self: looks like the buildInputs and nativeBuildInputs need to be in sync too, otherwise rustc will try to rebuild crates and fail because it is readonly.

I have robotica-slint working, will try to clean it up a bit tomorrow. And then try to work out how to deal with brian-robotica.

@ipetkov
Copy link
Owner

ipetkov commented Jul 6, 2023

@brianmay

robotica-rust is the top level package. Unless I have misunderstood something, I think the top level package is required to enable the sub packages.

The top level Cargo.toml at the root of the repository needs at least one of (but can include both):

  • a [package] definition
  • a [workspace] definition

Note that the [workspace] definition isn't a child of [package]; if [workspace] is the only thing defined then cargo will operate over the entire workspace by default (otherwise it will operate only on the [package] unless --workspace is passed in.


Wondering if I should be splitting some of this stuff into its own repo and/or workspace. Maybe at least for the wasm stuff. That way I don't have to worry wasm dependencies getting built for non-wasm builds.

I would definitely recommend splitting out wasm builds into their own separate set of derivations (you don't have to make it a separate repo to make it work with crane, but it's otherwise up to you). The compiler will emit totally different artifacts for different targets so there's no need to group x86 and wasm builds in the same derivation (otherwise changes to one will require rebuilding the other)


Note to self: looks like the buildInputs and nativeBuildInputs need to be in sync too, otherwise rustc will try to rebuild crates and fail because it is readonly.

This is correct, or rather if any dependencies need other build inputs (e.g. openssl, or pkg-config) those will need to be present in the buildDepsOnly derivation as well. I think a lot of *-sys crates try to fall back to building from source if the right libraries aren't available, but that usually runs into other errors.

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