feat: simplify git dependency detection by only using cargo lock #167

yusdacra · 2021-04-15T18:57:08Z

This PR also adds allRefs and submodules support. Since these features were added to builtins.fetchGit recently, they are not enabled by default.

mschwaig · 2021-05-08T13:19:58Z

I tested this branch to see if it works for fetching the submodules of https://crates.io/crates/tflite/0.9.5.

I set gitSubmodules = true but did not set allRefs = true;.

That works for those regular crates.io dependencies like this:

tflite = "0.9.5"

And the same attributes also work for git dependencies like this:

tflite = { git = "https://github.com/mschwaig/tflite-rs.git", branch = "stop-rewriting-flattbuffers-test" }

I think merging this PR would close #110 and that well-written issue was really helpful to me.

Before I found this PR I wanted to open a PR for this tiny change to fetch submodules for git dependencies only: mschwaig@2dd2f1b

tflite still fails to build with naersk for unrelated reasons, but I tested with that because that's what I'm working on

yusdacra · 2021-05-12T20:16:52Z

I tested this branch to see if it works for fetching the submodules of crates.io/crates/tflite/0.9.5.

I set gitSubmodules = true but did not set allRefs = true;.

That works for those regular crates.io dependencies like this:
tflite = "0.9.5"
And the same attributes also work for git dependencies like this:
tflite = { git = "https://github.com/mschwaig/tflite-rs.git", branch = "stop-rewriting-flattbuffers-test" }
I think merging this PR would close #110 and that well-written issue was really helpful to me.

Before I found this PR I wanted to open a PR for this tiny change to fetch submodules for git dependencies only: mschwaig@2dd2f1b

tflite still fails to build with naersk for unrelated reasons, but I tested with that because that's what I'm working on

Unrelated to this PR; but may I ask why and where tflite-rs errors?

mschwaig · 2021-05-12T21:50:49Z

Unrelated to this PR; but may I ask why and where tflite-rs errors?

It builds fine with nix develop, but fails to link when built with nix build.

You can take a closer look here if you are interested: https://github.com/mschwaig/tflite-rs-not-building

That's the minimal test case I got so far. Maybe I should open an issue for this, but I'm not sure. I have not tried building it without naersk yet.

nrdxp · 2021-09-20T23:58:23Z

Is there something holding this up? I've been using it fine for over a month now as I need it for direct git dependencies in a work project.

blitz · 2021-11-18T11:08:25Z

README.md

@@ -47,6 +47,8 @@ it is converted to an attribute set equivalent to `{ root = theArg; }`.
 | `version` | The version of the derivation. |
 | `src` | Used by `naersk` as source input to the derivation. When `root` is not set, `src` is also used to discover the `Cargo.toml` and `Cargo.lock`. |
 | `root` | Used by `naersk` to read the `Cargo.toml` and `Cargo.lock` files. May be different from `src`. When `src` is not set, `root` is (indirectly) used as `src`. |
+| `allRefs` | Whether to fetch all refs while fetching Git dependencies. Useful if the wanted revision isn't in the default branch. |
+| `gitSubmodules` | Whether to fetch submodules while fetching Git dependencies. |


If this maps to builtins.fetchGit's submodule support, we should note here that this only works for Nix 2.4+.

README.md

lib.nix

FlorianFranzen · 2021-12-23T13:50:43Z

I am a big fan of this PR, as it allowed me to only link actual dependencies when creating git-deps, which drastically reduces build times for my use case. See yusdacra#1 for details.

jwoudenberg · 2022-01-12T17:31:47Z

Just ran into #152. Switching to the naersk from this branch fixed things for me.

Without this the Nix build is currently failing, because the stable naersk has a bug related to using git dependencies in crate overrides, see: nix-community/naersk#152 Fixing to the naersk in this PR fixed things: nix-community/naersk#167 This should be a short-lived changed. I hope to either get my PR for the abomonation library merged which would make the crate override unnecessary. Should it not get merged I'll likely abandon my abomonation fork for some alternative solution.

Patryk27 · 2022-02-09T12:11:37Z

Thanks for this patch, it's working great!

I've found only one small issue - something on the way doesn't handle branches with / correctly, so trying to compile a patch such as this:

[patch."ssh://...."]
foo = { git = "ssh://...", branch = "foo-123/bar" }

... fails on a cp somewhere.

Edit: the fix turns out to be pretty simple!

In build.nix, near unpack(), just change:

      unpack() {
        toml=$1; nkey=$2

.... into:

      unpack() {
        toml=$1
        nkey=$2
        nkey=''${nkey/\//_}

(this sanitizes paths, so that foo-123/bar is stored nuder foo-123_bar, instead of inside a subdirectory.)

Patryk27 · 2022-03-17T17:35:09Z

@yusdacra thanks for this merge request - would you mind rebasing it? I'd love to merge it after that 🙂

…Refs support

Instead of unpacking all crates in a git dep, this only unpacks the crates specified in the cargo lock file.

Patryk27 · 2022-03-20T18:34:02Z

@yusdacra thanks! Would you also like to address the comments from @blitz? If you don't have time or something, it's fine - I can address them in a separate merge request myself 🙂

Patryk27 · 2022-03-28T16:53:36Z

@yusdacra could you tick Allow edits from maintainers? I'll then just push the changes into this pull request and get it merged 🙂

yusdacra · 2022-03-28T17:15:41Z

@yusdacra could you tick Allow edits from maintainers? I'll then just push the changes into this pull request and get it merged 🙂

It seems to already be ticked? Also sorry for not replying, I haven't really had time.

Patryk27 · 2022-03-28T17:28:46Z

It seems to already be ticked?

Sorry then, it looks like I've stumbled upon some bug in GitHub's permissions - I'll sort it out 🙂

I haven't really had time.

No worries, I understand; thanks!

Patryk27 · 2022-04-02T10:23:04Z

I'm having some github-permission-issues trying to push my changes in here, so I'm going to merge this pull request into a temporary, non-master branch, push my changes there, and then create a separate PR for that updated branch, hope you don't mind.

Thank y'all for reviews and see you in the updated PR (I'll post a link here when it's done) 🙂

Patryk27 · 2022-04-02T10:42:51Z

Follow-up: #231

yusdacra requested a review from nmattia as a code owner April 15, 2021 18:57

yusdacra force-pushed the feat/cargolock-git-deps branch 4 times, most recently from 9c3b87d to ae995d1 Compare April 22, 2021 19:46

yusdacra force-pushed the feat/cargolock-git-deps branch 13 times, most recently from 8f5f1e9 to 07d0b56 Compare April 25, 2021 00:55

yusdacra changed the title ~~feat: simplify git dependency detection by only using cargo lock, allRefs support~~ feat: simplify git dependency detection by only using cargo lock Apr 27, 2021

yusdacra force-pushed the feat/cargolock-git-deps branch from 07d0b56 to f411315 Compare June 13, 2021 04:25

blitz requested changes Nov 18, 2021

View reviewed changes

FlorianFranzen mentioned this pull request Dec 23, 2021

build: unpack only named git deps yusdacra/naersk#1

Merged

FlorianFranzen mentioned this pull request Jan 7, 2022

build: skip invalid manifests #220

Closed

yusdacra requested a review from Anderssorby as a code owner January 7, 2022 11:28

Patryk27 added a commit to anixe/naersk that referenced this pull request Mar 13, 2022

Add nix-community#167

61b26a4

yusdacra and others added 3 commits March 18, 2022 15:12

feat: simplify git dependency detection by only using cargo lock, all…

bd4822e

…Refs support

feat: optional submodule fetching

48bf5b2

build: unpack only named git deps (#1)

7882e30

Instead of unpacking all crates in a git dep, this only unpacks the crates specified in the cargo lock file.

yusdacra force-pushed the feat/cargolock-git-deps branch from 8616a0d to 7882e30 Compare March 18, 2022 12:13

Patryk27 mentioned this pull request Mar 28, 2022

tomlDependencies: pass allRefs=true when fetching a rev #211

Closed

Patryk27 mentioned this pull request Apr 1, 2022

Symlinks are not resolved after a submodule is cloned #230

Closed

Patryk27 changed the base branch from master to pr-167 April 2, 2022 10:21

Patryk27 merged commit 8187b80 into nix-community:pr-167 Apr 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: simplify git dependency detection by only using cargo lock #167

feat: simplify git dependency detection by only using cargo lock #167

yusdacra commented Apr 15, 2021 •

edited

Loading

mschwaig commented May 8, 2021 •

edited

Loading

yusdacra commented May 12, 2021

mschwaig commented May 12, 2021

nrdxp commented Sep 20, 2021

blitz Nov 18, 2021

FlorianFranzen commented Dec 23, 2021

jwoudenberg commented Jan 12, 2022

Patryk27 commented Feb 9, 2022 •

edited

Loading

Patryk27 commented Mar 17, 2022

Patryk27 commented Mar 20, 2022

Patryk27 commented Mar 28, 2022

yusdacra commented Mar 28, 2022

Patryk27 commented Mar 28, 2022

Patryk27 commented Apr 2, 2022

Patryk27 commented Apr 2, 2022

feat: simplify git dependency detection by only using cargo lock #167

feat: simplify git dependency detection by only using cargo lock #167

Conversation

yusdacra commented Apr 15, 2021 • edited Loading

mschwaig commented May 8, 2021 • edited Loading

yusdacra commented May 12, 2021

mschwaig commented May 12, 2021

nrdxp commented Sep 20, 2021

blitz Nov 18, 2021

Choose a reason for hiding this comment

FlorianFranzen commented Dec 23, 2021

jwoudenberg commented Jan 12, 2022

Patryk27 commented Feb 9, 2022 • edited Loading

Patryk27 commented Mar 17, 2022

Patryk27 commented Mar 20, 2022

Patryk27 commented Mar 28, 2022

yusdacra commented Mar 28, 2022

Patryk27 commented Mar 28, 2022

Patryk27 commented Apr 2, 2022

Patryk27 commented Apr 2, 2022

yusdacra commented Apr 15, 2021 •

edited

Loading

mschwaig commented May 8, 2021 •

edited

Loading

Patryk27 commented Feb 9, 2022 •

edited

Loading