rustPlatform: Allow impure cache. #98813

kevincox · 2020-09-25T23:58:24Z

I hope this isn't too controversial. Right now if you make any change to your rust source (including any non-source files) a subsequent Nix build will recompile all of your dependencies and do a full compile of your crate. This change allows using a directory of your choice to cache cargo's artifacts. This allows incremental rebuilds.

The intent of this is that it can be used in CI, and possibly local development to reduce full rebuilds. This is especially useful if you depend on a large number of crates.

This is not intended to be used for production releases and other cases where complete correctness is required.

Concerns:

I think executables that have been removed from the source will be grabbed by the installPhase so might not be removed from the package output.

Motivation for this change

Reduce development and CI time while being able to use the same Nix build process for CI and deployment.

Things done

I hope this isn't too controversial. Right now if you make any change to your rust source (including any non-source files) a subsequent Nix build will recompile all of your dependencies and do a full compile of your crate. This change allows using a directory of your choice to cache cargo's artifacts. This allows incremental rebuilds. The intent of this is that it can be used in CI, and possibly local development to reduce full rebuilds. This is especially useful if you depend on a large number of crates. This is not intended to be used for production releases and other cases where complete correctness is required. Concerns: - I think executables that have been removed from the source will be grabbed by the installPhase so might not be removed from the package output.

zowoq · 2020-09-26T00:59:01Z

Why do this rather than using buildRustCrate or one of the other rust/nix builders?

https://github.com/edolstra/import-cargo
https://github.com/kolloch/crate2nix
https://github.com/nmattia/naersk
https://github.com/tenx-tech/cargo2nix
Probably one or two others that I've forgotten.

kevincox · 2020-09-26T01:03:53Z

I didn't know about https://github.com/edolstra/import-cargo and all the rest require generating files instead just just using the regular cargo setup for development. I guess I'll have to check out the status of flakes to see if that one will work well.

However IIUC even in that case it won't cache the incremental build of the crate itself. However as long as I don't need to rebuild dependencies it will be fast enough for my CI.

kevincox · 2020-09-26T01:18:53Z

I also tested edolstra's and it doesn't appear to avoid recompiling dependencies (or I set it up wrong).

zowoq · 2020-09-26T04:23:21Z

... all the rest require generating files instead just just using the regular cargo setup for development.

Sorry but I still don't understand why you would want to use buildRustPackage if you want a "regular cargo setup"?

danieldk · 2020-09-26T06:20:17Z

I think executables that have been removed from the source will be grabbed by the installPhase so might not be removed from the package output.

cargo clean -p SPEC could be used to (only) clean the main package, which would avoid such left-overs.

I hope this isn't too controversial. Right now if you make any change to your rust source (including any non-source files)

I don't know. This feels like an invitation for people to disable sandboxing, which will lead to more breakage of Nix' model [1] and potentially more incorrect PR submissions. As @zowoq says, there are proper solutions to this. E.g. buildRustCrate+crate2nix build every transitive (crate) dependency as a Nix derivation, giving you fully incremental builds. As a bonus, build outputs are shared between projects. We have been using crate2nix with a binary cache in CI over year and it works great.

I would prefer to see crate2nix added to nixpkgs (ping @kolloch, any objections?) and steering people who need incremental builds in that direction.

[1] Which buildRustPackage already is, since it doesn't use proper fixed-output derivations.

andir · 2020-09-26T07:24:50Z

The author should look at naersk https://github.com/nmattia/naersk as that does a two stage compilation. One for the dependencies and another for your crate.

…

On Sat, Sep 26, 2020, 08:20 Daniël de Kok ***@***.***> wrote: * I think executables that have been removed from the source will be grabbed by the installPhase so might not be removed from the package output. cargo clean -p SPEC could be used to (only) clean the main package, which would avoid surch left-overs. I hope this isn't too controversial. Right now if you make any change to your rust source (including any non-source files) I don't know. This feels like an invitation for people to disable sandboxing, which will lead to more breakage of Nix' model [1] and potentially more incorrect PR submissions. As @zowoq <https://github.com/zowoq> mentions, there are proper solutions to this. E.g. buildRustCrate+crate2nix build every transitive (crate) dependency as a Nix derivation, giving you fully incremental builds. As a bonus, built outputs are shared between projects. We have been using crate2nix with a binary cache in CI over year and it works great. I would prefer to see crate2nix added to nixpkgs (ping @kolloch <https://github.com/kolloch>, any objections?) and steering people in that direction. [1] Which buildRustPackage already is, since it doesn't use proper fixed-output derivations. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#98813 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE365HXBYZOMRMI5OSB6LTSHWB23ANCNFSM4R2QOSEQ> .

danieldk · 2020-09-26T09:08:23Z

The author should look at naersk https://github.com/nmattia/naersk as that does a two stage compilation. One for the dependencies and another for your crate.

naerks is nice too, but the caveat is that all the dependencies are recompiled when Cargo.lock is modified. My experience is that even with modest projects using the async ecosystem (lots of dependencies), this generally leads to ~8 minute rebuilds on GitHub actions. For an early-stage project with quickly-changing dependencies I burned through my GH Actions minutes in a few days ;).

So I guess it depends a bit on your CI capacity and how often you expect to make changes that affect Cargo.lock.

(Yes, I should get an enterprise plan or set up my own GH Actions runner ;).)

andir · 2020-09-26T10:01:33Z

I think I'll finally work on a comparison blog post for all those approaches that I used in the past. There are always a few trade offs to make.

…

On Sat, Sep 26, 2020, 11:08 Daniël de Kok ***@***.***> wrote: The author should look at naersk https://github.com/nmattia/naersk as that does a two stage compilation. One for the dependencies and another for your crate. naerks is nice too, but the caveat is that all the dependencies are recompiled when Cargo.lock is modified. My experience is that even with modest projects using the async ecosystem (lots of dependencies), this generally leads to ~8 minute rebuilds on GitHub actions. For an early-stage project with quickly-changing dependencies I quickly burned through my GH Actions minutes in a few days ;). So I guess it depends a bit on your CI capacity and how often you expect to make changes that affect Cargo.lock. (Yes, I should get an enterprise plan or use my GH Actions runner ;).) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#98813 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE365A5FKV4ZIBG7LWPCBLSHWVRJANCNFSM4R2QOSEQ> .

danieldk · 2020-09-26T10:19:43Z

I think I'll finally work on a comparison blog post for all those approaches that I used in the past. There are always a few trade offs to make.

That would be really nice!

danieldk · 2020-09-26T10:23:02Z

I think I'll finally work on a comparison blog post for all those approaches that I used in the past. There are always a few trade offs to make.

That would be really nice!

The nixpkgs manual is also quite outdated when it comes to comparing approaches. E.g., it still suggest Carnix for buildRustCrate. As far as I can see, Carnix is not actively maintained anymore.

kevincox · 2020-09-26T11:05:22Z

and potentially more incorrect PR submissions

Just to be clear I don't think this option should ever be used in nixpkgs.

However there are cases where perfect purity are not worth the cost. This is why every CI system I am aware of except hydra has support for caching. Sure, it would be more correct if you built from source, but the time and resource cost isn't worth it.

kevincox · 2020-09-26T11:18:55Z

Sorry but I still don't understand why you would want to use buildRustPackage if you want a "regular cargo setup?

It makes the project approachable for people who don't use nix.
I can use tools like cargo-edit with no extra steps.
I can still use nix to declare and fetch my dependencies (very helpful for CI and also helps people who want to use nix locally)

My primary use case here is speeding up my CI. I have found nix very helpful in CI however full rebuilds are a source of pain. I have resorted to useing nix-shell --run cargo build but this means that I am bypassing a large part of the buuld I have defined, which looses features and leads to skew.

This PR makes an option that is very similar to a regular build with just the addition of allowing caching.

zowoq · 2020-09-26T11:41:17Z

I have resorted to useing nix-shell --run cargo build but this means that I am bypassing a large part of the buuld I have defined, which looses features and leads to skew.

Why not define the build once and import it into both nix-shell and buildRustPackage?

danieldk · 2020-09-26T12:28:38Z

pkgs/build-support/rust/default.nix

+  releaseDirPrefix = if impureCacheDir == null
+    then "target"
+    else
+      assert
+        stdenv.lib.assertMsg (stdenv.lib.hasPrefix "/" impureCacheDir)
+        "impureCacheDir must be an absolute path.";
+      impureCacheDir;


Maybe impureCacheDir could just be renamed cacheDir (or perhaps targetDir) with the default set to "target"? Then this absolute path check is not necessary.

I think this would be a good compromise, because then the derivation does not encourage impure use, but anyone who is willing to disable the Nix sandbox could still set it to an absolute directory. I guess setting CARGO_TARGET_DIR to target as a result does not have a negative impact, except for causing a lot of rebuilds ;).

Maybe impureCacheDir could just be renamed cacheDir with the default set
to "target"? Then this absolute path check is not necessary.

That could be done.

...because then the derivation does not encourage impure use
I'm not sure. This just sounds like hiding the true purpose because I can't
think another reason to set the cache directory.

One possible alternative would be allow passing in the cache directory
manually and provide the resulting cache directory as a separate output.
This is weirder to use and technically pure. However I decided against this
solution as it didn't seem to add any real value other than working without
sandboxing.

Mic92 · 2020-09-27T11:36:44Z

pkgs/build-support/rust/default.nix

+# If set the CARGO_TARGET_DIR will be set to this directory. This will allow
+# you to avoid rebuilding dependencies.
+# This setting is *incompatible* with sandboxing.
+, impureCacheDir ? null


This also should be in the manual, ideally with an example. I assume one could use a relaxed sandbox, where only the rust package is built impure (__noChroot = true;)

I checked the docs and there isn't an obvious place to put this as there is no attribute reference. Is there a spot that you would recomend?

kevincox · 2020-11-02T14:18:29Z

I'm closing this for now because naersk was good enough for me. However I think the approach is still viable when impute benefits such as caching are desired.

kevincox requested review from illegalprime, Ma27 and zowoq September 25, 2020 23:58

kevincox requested a review from andir as a code owner September 25, 2020 23:58

kevincox self-assigned this Sep 25, 2020

zowoq requested review from Mic92 and danieldk September 26, 2020 00:58

ofborg bot added 6.topic: rust 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Sep 26, 2020

danieldk reviewed Sep 26, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

Mic92 reviewed Sep 27, 2020

View reviewed changes

kevincox closed this Nov 2, 2020

kevincox deleted the kevincox-rust-cache branch November 2, 2020 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustPlatform: Allow impure cache. #98813

rustPlatform: Allow impure cache. #98813

kevincox commented Sep 25, 2020

zowoq commented Sep 26, 2020

kevincox commented Sep 26, 2020

kevincox commented Sep 26, 2020

zowoq commented Sep 26, 2020 •

edited

danieldk commented Sep 26, 2020 •

edited

andir commented Sep 26, 2020 via email

danieldk commented Sep 26, 2020 •

edited

andir commented Sep 26, 2020 via email

danieldk commented Sep 26, 2020

danieldk commented Sep 26, 2020 •

edited

kevincox commented Sep 26, 2020

kevincox commented Sep 26, 2020

zowoq commented Sep 26, 2020

danieldk Sep 26, 2020 •

edited

kevincox Sep 29, 2020

This comment has been minimized.

Mic92 Sep 27, 2020

kevincox Sep 29, 2020

kevincox commented Nov 2, 2020

rustPlatform: Allow impure cache. #98813

rustPlatform: Allow impure cache. #98813

Conversation

kevincox commented Sep 25, 2020

Motivation for this change

Things done

zowoq commented Sep 26, 2020

kevincox commented Sep 26, 2020

kevincox commented Sep 26, 2020

zowoq commented Sep 26, 2020 • edited

danieldk commented Sep 26, 2020 • edited

andir commented Sep 26, 2020 via email

danieldk commented Sep 26, 2020 • edited

andir commented Sep 26, 2020 via email

danieldk commented Sep 26, 2020

danieldk commented Sep 26, 2020 • edited

kevincox commented Sep 26, 2020

kevincox commented Sep 26, 2020

zowoq commented Sep 26, 2020

danieldk Sep 26, 2020 • edited

Choose a reason for hiding this comment

kevincox Sep 29, 2020

Choose a reason for hiding this comment

This comment has been minimized.

Mic92 Sep 27, 2020

Choose a reason for hiding this comment

kevincox Sep 29, 2020

Choose a reason for hiding this comment

kevincox commented Nov 2, 2020

zowoq commented Sep 26, 2020 •

edited

danieldk commented Sep 26, 2020 •

edited

danieldk commented Sep 26, 2020 •

edited

danieldk commented Sep 26, 2020 •

edited

danieldk Sep 26, 2020 •

edited