nix-community / vgo2nix

Convert go.mod files to nixpkgs buildGoPackage compatible deps.nix files [maintainer=@adisbladis]
MIT License
90 stars 21 forks source link

(Better) support for modern go.mod #40

Closed dsx closed 3 years ago

dsx commented 4 years ago
  1. Enable shallow clones. Big repos could literally take an hour to download on slow links.
  2. FWIW I tested this on github.com/rancher/k3s and it worked.
matt-snider commented 4 years ago

Hey @dsx I see that you also made some changes to the replace mechanism, which is broken as per #22 . Have you seen #36? I tried out your branch and I ran into the same issues reported there with invalid references.

panic: Error processing import path "github.com/docker/docker-credential-helpers":
nix-prefetch-git --fetch-submodules --url https://github.com/docker/docker-credential-helpers --rev 000000000000 failed:
// ...
fatal: '000000000000' is not a commit and a branch 'fetchgit' cannot be created from it
Unable to checkout 000000000000 from https://github.com/docker/docker-credential-helpers.

I'm also curious about the differences in the data structure in the two approaches. In #36 goMod.Replace is a different struct (goModReplacement) and non-recursive. Defining it as just a goMod as you've done seems clever, but I'm not familiar enough with go dependency management to know whether it makes sense for a Replace itself to have a Replace, as your data structure would permit.

dsx commented 4 years ago

Hi @matt-snider.

No, I haven't seen #36 before I started.

As far as I understand go dependency management, you kind of layering your replacements on top one another incrementally until you resolve final package name and version, hence the recursive structure.

This PR is more of a hack to get some things work. I'm sure there are way more cases which I didn't think of. Better approach is needed.

matt-snider commented 4 years ago

@dsx Thanks for the response. Any comment on your branch not working with some replace statements (e.g. ProtonMail/proton-bridge)? Is that out of scope or something that this PR might be able to address?

dsx commented 4 years ago

@matt-snider I'll try to find time next week to take a look at why proton-bridge acts funny.

dsx commented 4 years ago

@matt-snider Just tried to run my branch on proton-bridge master, no errors reported.

matt-snider commented 4 years ago

@dsx Thanks for taking a look. You are right, my apologies, I must have built your branch incorrectly or used the wrong version. It successfully generates the deps.nix! :tada:

Well in that case it seems to handle at least one case that #36 doesn't. @Lucas16 What do you think of this PR as compared to yours? Are there any edge cases you've run into that might be useful in evaluating this implementation?

matt-snider commented 4 years ago

@dsx Another thing I noticed. When using the example of the proton-bridge repository, it seems like the dep.nix file ends up with the wrong goPackagePath.

E.g. we have the replacement:

github.com/jameskeane/bcrypt => github.com/ProtonMail/bcrypt v0.0.0-20170924085257-7509ea014998

And in deps.nix:

  {
    goPackagePath = "github.com/ProtonMail/bcrypt";
    fetch = {
      type = "git";
      url = "https://github.com/ProtonMail/bcrypt";
      rev = "7509ea014998";
      sha256 = "12xi8i4sb6q4h4wd6w1phqpzxpff5c629ard8cnkjp7qmznvcc20";
    };
  }

Shouldn't it be goPackagePath = "github.com/jameskeane/bcrypt"?

dsx commented 4 years ago

@matt-snider ProtonMail maintains own fork of github.com/jameskeane/bcrypt and wants go compiler to use their fork (github.com/ProtonMail/bcrypt) wherever original bcrypt is used. Semantics are the same as those of cp <from> <to> .

Therefore dep.nix is correct.

matt-snider commented 4 years ago

@dsx I agree with your description of how ProtonMail is using replace, but to use your cp analogy, the <from> is missing in the generated deps.nix. We want github.com/jameskeane/bcrypt to be replaced by github.com/ProtonMail/bcrypt, so we need goPackagePath = "github.com/jameskeane/bcrypt", don't we?

I am not very familiar with go, but it seems the test from #36 also suggests this is correct (i.e. replace golang.org/x/text => github.com/golang/text)

dsx commented 4 years ago

Nix doesn't need to know what's up with replacements in go.mod. All it has to know is that one way or another github.com/ProtonMail/bcrypt will be required at some point. There is no need to reference github.com/jameskeane/bcrypt because at no point this package is being used for building resulting binary.

SuperSandro2000 commented 3 years ago

@Mic92 this can probably be closed, too.