-
Notifications
You must be signed in to change notification settings - Fork 85
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
Handle custom registries #214
base: master
Are you sure you want to change the base?
Conversation
@Anderssorby Looks pretty uncontroversial. Is there something in the test suite that tests this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'm a bit rusty on how everything here works, so letting @blitz approve officially!
I would say if the old workflow is still correct, then we should merge. At a glance, I don't see anything wrong. If something is wrong, then someone else can create an issue, and provide feedback as to how it's now working in their use case. @Anderssorby do you mind squashing the fixup commit? |
Update lib.nix Co-authored-by: Jonathan Ringer <[email protected]>
ac08a33
to
46598fa
Compare
@blitz So should we merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blitz So should we merge this?
The code looks good, but one of the "slow" tests (as opposed to fast tests we execute in the CI) are still failing:
$ nix-build test.nix --show-trace -A rustlings
error: please specify either 'src' or 'root'
… while evaluating 'resolveSource'
at /home/julian/src/own/naersk/lib.nix:14:19:
13| # not defined.
14| resolveSource = attrs:
| ^
15| let
… from call site
at /home/julian/src/own/naersk/default.nix:28:16:
27| # Config for cargo itself
28| source = libb.resolveSource arg;
| ^
29| configFile = source.root + "/.cargo/config.toml";
… while evaluating 'optionalString'
at /nix/store/vqd0zjq6qjs5zz2m1nnzp4w5glf7ga7r-nixpkgs-src/lib/strings.nix:202:5:
201| # String to return if condition is true
202| string: if cond then string else "";
| ^
203|
… from call site
at /home/julian/src/own/naersk/lib.nix:228:15:
227| mkdir -p $out/.cargo
228| ${lib.optionalString (! isNull cargoConfigText) "cp ${config} $out/.cargo/config"}
| ^
229| cp ${cargolock'} $out/Cargo.lock
… while evaluating the attribute 'buildCommand' of the derivation 'dummy-src'
at /nix/store/vqd0zjq6qjs5zz2m1nnzp4w5glf7ga7r-nixpkgs-src/pkgs/stdenv/generic/make-derivation.nix:205:7:
204| // (lib.optionalAttrs (attrs ? name || (attrs ? pname && attrs ? version)) {
205| name =
| ^
206| let
… while evaluating the attribute 'src' of the derivation 'rustlings-deps-1.3.0'
at /nix/store/vqd0zjq6qjs5zz2m1nnzp4w5glf7ga7r-nixpkgs-src/pkgs/stdenv/generic/make-derivation.nix:205:7:
204| // (lib.optionalAttrs (attrs ? name || (attrs ? pname && attrs ? version)) {
205| name =
| ^
206| let
… while evaluating the attribute 'builtDependencies' of the derivation 'rustlings-1.3.0'
at /nix/store/vqd0zjq6qjs5zz2m1nnzp4w5glf7ga7r-nixpkgs-src/pkgs/stdenv/generic/make-derivation.nix:205:7:
204| // (lib.optionalAttrs (attrs ? name || (attrs ? pname && attrs ? version)) {
205| name =
| ^
206| let
</details>
Hi, @Anderssorby - thanks for this merge request! Would you mind rebasing it & addressing the comment from @blitz? |
Sure, I can take a look later. |
@Anderssorby Awesome work. I would also like to see this feature get merged in. Is there anything I can do to help you close out this PR? |
Hello, I would also like to see this feature merged and I tried to understand why the test wasn't passing.
instead of this:
There are two ways to fix this issue:
Let me know if I can contribute further |
Thanks for analyzing the issue, your second approach seems the most fitting - I'll try to rebase this pull request with your patch applied over the weekend 🙂 |
Hmm, maybe I'm misunderstanding something, but the solution presented in this pull request can't work, can it? 👀
So, say, we're given a [[package]]
name = "dep"
version = "0.1.0"
source = "registry+https://git.some-company.com/rust/crates-index"
checksum = "bae3d8de1b7fd1fef6c2da3130a7d06d32499fd5292a9c1309681ac79e98c643" To actually download that package naresk would have to:
... and I think the first step is fundamentally problematic in Nix, since we don't know at which commit we should checkout that hypothetical I'd say that in order to have some minimum viable product we should extend { ... }:
naersk.buildPackage {
src = ./.;
registries = {
# crates-io remains implied, ofc.
custom = {
dl = "https://my-registry.io/api/v1/crates/{version}/{crate}/whatever/{lowerprefix}";
};
};
} I think this would solve all of the problems here, but I'm leaving this one up to discussion 🙂 Edit: I see that Crane uses a similar solution 😄 (https://github.com/ipetkov/crane/blob/5548a68f5d4d60a2315ab1232e6fba5528e71dc8/examples/alt-registry/flake.nix#L35) |
So I started looking into it again and have an implementaton of buildPackage = args@{registries? {}, ... }: that passes the fast tests but fails the slow ones:
should I just make a |
To create a new parameter, you should just add it to |
Hi! What is missing to get this PR through? I'm the maintainer of https://kellnr.io and I would really like to be able to use a private registry for Rust with Nix. Unfortunately, I'm very new to Nix, so I lack the skill to implement the feature myself. |
Oh wow, Crane looks nice! Since it's maintained, well documented and supports many more features I think it's best to archive naersk and point people to Crane; best if people don't try setting up naersk just to realize it's deprecated anyway. Unless people complain I'll do that soon! |
To fix #186