feat(rust): implement crane builder #83

yusdacra · 2022-02-13T19:05:28Z

This PR adds a crane builder. Crane builds a dependencies only derivation first, and then the actual crate. For this reason, I have made the deps only derivation use ${pname}-deps as override name and the actual crate just uses pname like normal.

flake.nix

yusdacra · 2022-02-15T22:03:44Z

src/builders/default.nix

@@ -21,8 +21,10 @@
  };

  rust = rec {
-    default = buildRustPackage;
+    default = crane;


Should the default be made crane?

I leave that decision up to you.

I made it the default builder since it allows for more caching than buildRustPackage. It should be a sane default if we fix the IFD woes. It also seemed much simpler to me than how buildRustPackage reads / works, so I think it would be more easy to override.

yusdacra · 2022-02-15T22:06:26Z

src/default.nix

@@ -238,6 +298,7 @@ let
  # build a dream lock via a specific builder
  callBuilder =
    {
+      source,


This is needed to pass the original source the user passed to riseAndShine so that it can be used in the crane builder, because crane expects a Cargo.lock and a Cargo.toml for a builds only dep, which will only be available at workspace root for workspaces. dream2nix dependency sources won't have this information if it's a workspace, of course.

Maybe the name could be changed to something else though?

Hm, but this will only be available if riseAndShine is called with a source tree as source. But it must also work if a dream-lock.json is passed as source. Therefore all necessary information for building must be in the dream-lock.

Actually since the dream-lock now supports more than one top-level package, we need to extend the path source type with a field that points to the root package source it resides in.
We could add for example a field named root pointing to a package name from .generic.packages.
Then we could add a builder function getSourceRoot or something like that.

What do you think?

Yes, it is kind of hacky right now. It does try to handle it if it's a dream-lock but it's not good with that.

A getSourceRoot function that I would need should essentially just "resolve" path dependencies to their root, so without the relative path prepended. Everything else is a "root" on their own, so they don't matter.

Would the getSourceRoot function even be sufficient for this? Or would there still be the problem that we don't know the exact location of the toml file and have to search for it again?

Another potential problem: Does crane read the cargo toml/lock during eval time?
If we build from dream-lock, then reading contents of files within the source would require IFD, because the source has to be fetched first, for evaluation to continue. We should prevent that.

In case we really need the Cargo.toml content during eval time, we can embed the toml content inside subsystemAttrs and then pass it to the builder. Not nice, but the architecture would be fine with it.

getSourceRoot should be enough, it will always contain a Cargo.lock. For path dependencies this means a workspace, for others they are either fetched from crates-io which will always have a Cargo.lock or they are fetched from github / gitlab but aren't a workspace.

IIRC the only place crane does read the Cargo.toml is for the cleanCargoToml function which is called when building the dummy source. I believe that could be moved to the dummy source derivation itself, can make an issue for it on the crane repo. I think it also does IFD while generating some dummy source files. It would be appreciated if you could check lib/mkDummySource.nix and lib/cleanCargoToml.nix on crane repo for IFDs.

To be honest I think I would prefer the custom solution. Part of the idea behind dream2nix was to go away from the monolithic lock-file -> nix derivations model which most other 2nix tools use, and instead separate the process in distinct phases (translation, fetching, building), so we can have generic (user) interfaces for all of these phases.

I'm worried now that if we circumvent that model and allow exceptions, that later we might run into trouble keeping our interfaces uniform and shift more complexity to the end user.
But also I'm currently unable to come up with a good example that underlines this point.

So maybe we should just go ahead and merge this if you think this already adds value to the project. I leave the decision up to you.

If we decide to keep this, then we should at least remove the source attribute being passed to the builder and add the getSourceRoot function we talked about.

I'm not sure if I want to merge it since unlike buildRustPackage it doesn't use env variables in it's build / check / install phases which makes it hard to override those phases or some other stuff. buildRustPackage is fine for now IMO, until a granular builder lands.

I'll still keep the branch around though, in case someone has a better idea on how to do all this.

I think we can still merge it though, I feel it would be better if people saw something like this is actually implemented, some people might still want a deps only drv for better caching.

I can add a warning that this requires IFD, and not make it the default builder. And also introduce getSourceRoot. How does that sound?

Sounds great! Let's do it.

I implemented what you said on the matrix room, and it seems to be working fine now! Both buildRustPackage and crane builder seem to be able to build things fine. buildRustPackage is still the default. I added a comment in src/builders/default.nix for crane that says it requires IFD. Should be ready to go now!

DavHau

Thanks, please see comments below.

flake.nix

DavHau · 2022-02-17T05:06:08Z

src/builders/default.nix

@@ -21,8 +21,10 @@
  };

  rust = rec {
-    default = buildRustPackage;
+    default = crane;


I leave that decision up to you.

src/default.nix

DavHau · 2022-02-17T05:29:26Z

src/default.nix

@@ -238,6 +298,7 @@ let
  # build a dream lock via a specific builder
  callBuilder =
    {
+      source,


Hm, but this will only be available if riseAndShine is called with a source tree as source. But it must also work if a dream-lock.json is passed as source. Therefore all necessary information for building must be in the dream-lock.

Actually since the dream-lock now supports more than one top-level package, we need to extend the path source type with a field that points to the root package source it resides in.
We could add for example a field named root pointing to a package name from .generic.packages.
Then we could add a builder function getSourceRoot or something like that.

What do you think?

…he dependency drv

…ne builder

… drv

…changes by upstream crane, share some stuff between deps - main drv

…oisy

fix: remove source arg fix: remove more source args fix(rust/crane): remove unused logic temp

yusdacra force-pushed the rust/crane-builder branch 3 times, most recently from ac8cee2 to 1c4b8aa Compare February 13, 2022 19:29

yusdacra marked this pull request as ready for review February 13, 2022 21:03

yusdacra force-pushed the rust/crane-builder branch 3 times, most recently from d1572ee to 1a73712 Compare February 15, 2022 19:36

yusdacra commented Feb 15, 2022

View reviewed changes

flake.nix Outdated Show resolved Hide resolved

yusdacra commented Feb 15, 2022

View reviewed changes

yusdacra mentioned this pull request Feb 16, 2022

stack overflow with a certain Rust project #86

Closed

DavHau reviewed Feb 17, 2022

View reviewed changes

yusdacra force-pushed the rust/crane-builder branch 2 times, most recently from dd0c1d2 to 1de1753 Compare February 18, 2022 23:48

yusdacra mentioned this pull request Feb 20, 2022

fix(rust): some rust builder improvements #88

Merged

yusdacra force-pushed the rust/crane-builder branch from cea574f to 0cd3692 Compare February 20, 2022 13:54

yusdacra mentioned this pull request Feb 24, 2022

packaging ripgrep: not all packages build #91

Closed

DavHau force-pushed the main branch from 5b39b72 to 0c7e66d Compare February 25, 2022 09:44

yusdacra force-pushed the rust/crane-builder branch 12 times, most recently from 8bc6966 to df109c6 Compare February 27, 2022 16:57

yusdacra added 25 commits February 27, 2022 21:23

feat(rust): implement initial crane builder

681c973

feat(rust/crane): use pname-deps instead of pname for overrides for t…

ea294c5

…he dependency drv

chore(deps): update crane flake dep

71eb8de

fix(rust/crane): writeTOML is not from pkgs set

cdabbfc

fix(rust/crane): use original source if available

b9a0b41

chore(rust/crane): add comment

b214768

fix(rust): move vendored git deps config to vendor.nix, use it in cra…

f8a3d27

…ne builder

fix(rust): use nix-sources as vendored sources name for crane builder

5e0378a

fix(rust/crane): also write vendored git deps to cargo config on main…

8ec2e4a

… drv

refactor(rust/crane): explicitly set deps drv name suffix to prevent …

dc7d841

…changes by upstream crane, share some stuff between deps - main drv

feat(rust/cargo-lock): remove binary not found warning as it is too n…

566474b

…oisy

feat(rust/crane): vendor our own toTOML instead of pulling in nix-std

2265819

feat(rust/crane): use buildPackage to build

7d89e01

fix(rust/crane): use correct hooks in mkCargoDerivation

7921dc3

fix(rust/crane): fix func arguments

4d6ec91

feat(rust): switch back to buildRustPackage as default rust builder

4650685

feat: allow rootName and rootVersion in path source type

bb09404

fix(rust/crane): remove source arg

1054a4f

fix: remove source arg fix: remove more source args fix(rust/crane): remove unused logic temp

fix(rust/buildRustPackage): pass cargoBuildFlags with correct flags

e50466b

fix(rust): implement getRoot to get root pname version

5942119

fix(rust): allow passing other args

2e32b4f

fix: actually pass getRoot to builders

30b8720

fix(rust): getRootSource -> getRoot

5b18525

docs(rust): add ifd warning to crane builder

309af8d

fix(rust/crane): make sure cargo only builds the package we want

2fe01e1

yusdacra force-pushed the rust/crane-builder branch from df109c6 to 2fe01e1 Compare February 27, 2022 18:23

feat: expose builders in lib.init

c580c24

DavHau merged commit 4656834 into nix-community:main Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust): implement crane builder #83

feat(rust): implement crane builder #83

yusdacra commented Feb 13, 2022 •

edited

yusdacra Feb 15, 2022

DavHau Feb 17, 2022

yusdacra Feb 17, 2022

yusdacra Feb 15, 2022

DavHau Feb 17, 2022

yusdacra Feb 17, 2022

DavHau Feb 17, 2022 •

edited

yusdacra Feb 17, 2022

DavHau Feb 20, 2022

yusdacra Feb 20, 2022

yusdacra Feb 20, 2022

DavHau Feb 21, 2022

yusdacra Feb 27, 2022

DavHau left a comment

DavHau Feb 17, 2022

DavHau Feb 17, 2022

feat(rust): implement crane builder #83

feat(rust): implement crane builder #83

Conversation

yusdacra commented Feb 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavHau Feb 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavHau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusdacra commented Feb 13, 2022 •

edited

DavHau Feb 17, 2022 •

edited