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 to pass outputHashes to crane #266

Merged
merged 9 commits into from
Sep 22, 2023

Conversation

Mic92
Copy link
Contributor

@Mic92 Mic92 commented Mar 14, 2023

This makes it possible to evaluate crane in a nixos test without network
as well as allow to backup all fetched input derivations properly in a
binary cache, whereas fetchGit will fallback to downloading from a
repository, which also requires a git binary to be present.

Motivation

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

@Mic92
Copy link
Contributor Author

Mic92 commented Mar 14, 2023

If you agree on the idea I can work on tests/docs for this.

@Mic92
Copy link
Contributor Author

Mic92 commented Mar 14, 2023

Currently this is what the api looks like:

  cargoArtifacts = craneLib.buildDepsOnly {
   inherit src buildInputs nativeBuildInputs cargoExtraArgs;
   outputHashes = {
     "https://github.com/youruser/yourrepo?rev=4c220d364c53138d7dc37c6c95fee4b9f2efdc77" = "sha256-6DrNnzy2jYpkxiNReAkUl22Iz6au0+kmePmTXQxUsug=";
   };
 };

@Mic92 Mic92 closed this Mar 14, 2023
@Mic92 Mic92 reopened this Mar 14, 2023
@Mic92 Mic92 marked this pull request as draft March 14, 2023 14:56
@Mic92 Mic92 force-pushed the allow-output-hashes branch 7 times, most recently from fa6f2cc to b21c96e Compare March 14, 2023 15:27
@Mic92 Mic92 marked this pull request as ready for review March 14, 2023 15:30
Copy link
Owner

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this @Mic92! I had assumed that builtins.fetchGit would work offline since we already specify a git revision, but I suppose Nix wants to use its own hash of the content itself and not the git hash 🤦‍♂️

Anyway I'm definitely open to including this (modulo adding tests and docs, but I can also help flesh those out as needed)!

Thinking from a broader API perspective it would be cool to allow dropping some kind of crane-git-hashes.json file next to Cargo.lock and having it automatically be picked up during vendoring so that consumers don't have to manually plumb through a custom vendorCargoDeps invocation. Though we can and should do that in a separate PR, we can focus on the lower level API changes fist

lib/vendorGitDeps.nix Outdated Show resolved Hide resolved
lib/vendorGitDeps.nix Outdated Show resolved Hide resolved
@Mic92 Mic92 force-pushed the allow-output-hashes branch 10 times, most recently from ff0f209 to d5c6bf6 Compare July 24, 2023 08:54
This makes it possible to evaluate crane in a nixos test without network
as well as allow to backup all fetched input derivations properly in a
binary cache, whereas fetchGit will fallback to downloading from a
repository, which also requires a `git` binary to be present.
@Mic92 Mic92 force-pushed the allow-output-hashes branch from d5c6bf6 to 956f4aa Compare July 24, 2023 09:01
@Mic92
Copy link
Contributor Author

Mic92 commented Jul 24, 2023

Thinking from a broader API perspective it would be cool to allow dropping some kind of crane-git-hashes.json file next to Cargo.lock and having it automatically be picked up during vendoring so that consumers don't have to manually plumb through a custom vendorCargoDeps invocation. Though we can and should do that in a separate PR, we can focus on the lower level API changes fist

Yes. Let's keep this simple. Json is probably also something consumers wouldn't want to write by hand.

A package's `source` attribute in `Cargo.lock` is unique because it
corresponds to an exact commit. Using the human-specified URL can result
in collisions (e.g. a workspace specifies the same "main" URL but locked
to two different commits)
@ipetkov
Copy link
Owner

ipetkov commented Jul 29, 2023

Thanks for addressing the feedback @Mic92, looking good!

I added a simple test case, and I also changed the key format of outputHashes to use the value of the package's source attribute in Cargo.lock. This version is actually unique since it includes the commit revision and makes it plainly obvious which hash maps to what entry in Cargo.lock if someone has to debug mismatched hashes! This should also make it a bit easier to write some kind of script/tool which can automatically generate output hashes from a Cargo.lock file but we can work on this afterwards.

The last bits to take care of before merging is to update the CHANGELOG and add some documentation around the change. Feel free to take a shot at that, but if not I'll come back to it later!

@ipetkov ipetkov enabled auto-merge (squash) September 22, 2023 03:31
@ipetkov ipetkov merged commit 16f5732 into ipetkov:master Sep 22, 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.

2 participants