Skip to content

buildGoPackage: move goDeps from JSON to nix#17254

Closed
kamilchm wants to merge 5 commits intoNixOS:masterfrom
kamilchm:go2nix
Closed

buildGoPackage: move goDeps from JSON to nix#17254
kamilchm wants to merge 5 commits intoNixOS:masterfrom
kamilchm:go2nix

Conversation

@kamilchm
Copy link
Member

@kamilchm kamilchm commented Jul 25, 2016

Motivation for this change

After #16017 there was a lot of comments saying that nix would be better
than JSON for Go packages dependency sets.
As said in #16017 (comment):

Because of the content-addressable store, if two programs have the
same dependency it will already result in the same derivation in the
store. Git also has compression in the pack files so it won't make
much difference to duplicate the dependencies on disk. And finally
most users will just use the binary builds so it won't make any
differences to them.

This PR removes all deps.json and libs.json files by replacing
it witth standard deps.nix dependecy set files that can be
generated by go2nix.

TODOs:

  • documentation
  • go2nix update
Things done
  • Tested using sandboxing
    (nix.useChroot on NixOS,
    or option build-use-chroot in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

CC: @zimbatm @offlinehacker @groxxda @rushmorem @lethalman @edolstra @chris-martin @dguibert @bjornfor @obadz

@mention-bot
Copy link

@kamilchm, thanks for your PR! By analyzing the annotation information on this pull request, we identified @zimbatm, @carlsverre and @vrthra to be potential reviewers

Copy link
Contributor

Choose a reason for hiding this comment

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

x/crypto is duplicated in this deps file -- might want to check elsewhere for similar issues

Copy link
Member Author

Choose a reason for hiding this comment

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

thx, I'll try find all duplcates and fix it

@carlsverre
Copy link
Contributor

I am down with the goals of this change. By having go-binaries reference their dependencies + versions it will assist with keeping go-binaries working. I feel that this change calls into question the purpose of pkgs/development/go-modules/libs.json. From now on the only purpose of that file is for go development environments. I would argue that most dev environments would have a particular set of versions they care about that shouldn't change just because the user updates nixpkgs. My preference is to remove that file and force people to handle dependencies themselves.

Are there any docs that should be updated with this change? Specifically docs which reference go2nix and/or how to write a go-binary package.

Also - question for @kamilchm - Are the versions of go dependencies the same as those in pkgs/development/go-modules/libs.json or are they the current revision of master from the source repo? IMO they should be the same as libs.json so this change doesn't introduce any weird bugs.

@kamilchm
Copy link
Member Author

kamilchm commented Jul 25, 2016

The lbs.json will be removed with this change. I didn't think deeply about development environments at this time. I want to clean up binary packages before and then move to the second case of go2nix, that will be development environments. I was thinking about something like godep where you just use go get to install packages and godep save to freeze version. I think that go2nix save could replace godep there and freeze package versions into deps.nix.

Current docs http://hydra.nixos.org/build/36798894/download/1/nixpkgs/manual.html#sec-language-go needs to be updated if we agree on this change.

All deps.nix files were generated from libs.json with the same versions using https://github.com/kamilchm/go2nix/blob/deps-nix/depsjson2nix.nix so the binaries should remain unchanged

go2nix from a branch https://github.com/kamilchm/go2nix/tree/deps-nix can generate Go packages in the new format.

@carlsverre
Copy link
Contributor

Nice, thanks for pushing this! Looking forward to it becoming the new way to manage go packages. LGTM

@avnik
Copy link
Contributor

avnik commented Jul 25, 2016

FYI: would be nice, if go2nix would be able handle godeps and glide manifests, and simpe convert them to deps.nix

@kamilchm
Copy link
Member Author

@avnik you can't generate deps.nix without sha of all package dependencies. Both Glide and Godep doesn't provide any checksums other than git rev.
So if you want to generate deps.nix for any of these you still have to download all packages with those tools. And if you do it all the deps will be available under current GOPATH so you could use go2nix save then without any special care for glide or godep.

@avnik
Copy link
Contributor

avnik commented Jul 25, 2016

On Mon, Jul 25, 2016 at 11:07:50AM -0700, Kamil Chmielewski wrote:

@avnik you can't generate deps.nix without sha of all package dependencies. Both Glide and Godep doesn't provide any checksums other than git rev.
So if you want to generate deps.nix for any of these you still have to download all packages with those tools. And if you do it all the deps will be available under current GOPATH so you could use go2nix save then without any special care for glide or godep.

@kamilchm It solely about packaging convinience, so I actually need download all stuff twice, firstly with go get/glide/godep, and secondly with go2nix (and fetchFromGitHub and
other nix stuff usually much faster than go-get. But it only sidenote/wishlist, not requirement and even not suggestion ;)

kamilchm added 3 commits July 26, 2016 06:41
After NixOS#16017 there were a lot
of comments saying that `nix` would be better than `JSON`
for Go packages dependency sets.
As said in
NixOS#16017 (comment)

> Because of the content-addressable store, if two programs have the
> same dependency it will already result in the same derivation in the
> store. Git also has compression in the pack files so it won't make
> much difference to duplicate the dependencies on disk. And finally
> most users will just use the binary builds so it won't make any
> differences to them.

This PR removes all `deps.json` and `libs.json` files by replacing
it witth standard `deps.nix` dependecy set files that can be
generated by `go2nix`.
@zimbatm
Copy link
Member

zimbatm commented Jul 30, 2016

What if the dependencies are fetched over mercurial or bazaar? With that approach the user need to coordinate the import inherited variables with what deps.nix needs. Maybe make the arguments in the deps.nix open-ended (eg: { fetchgit, ... }:) and then have a utility that imports and injects all the supported fetchers all the time. Actually if you have a helper you can also keep the deps.nix pure and compose the srcs from that at runtime.

@kamilchm
Copy link
Member Author

kamilchm commented Aug 1, 2016

@zimbatm in the current form you need to make sure that proper fetches are propagated from default.nix. go2nix helps you to introduce and update packages, but you as a maintainer need to fine tune it.

If I understand you correctly you want deps.nix to be pure like:

[
  {
    goPackagePath = "bitbucket.org/ww/goautoneg";
    rev = "75cd24fc2f2c2a2088577d12123ddee5f54e0675";
    sha256 = "19khhn5xhqv1yp7d6k987gh5w5rhrjnp4p0c6fyrd8z6lzz5h9qi";
  }
  {
    goPackagePath = "golang.org/x/crypto";
    rev = "1f22c0103821b9390939b6776727195525381532";
    sha256 = "1acy12f396sr3lrnbcnym5q72qnlign5bagving41qijzjnc219m";
  }
]

and make an utility in buildGoPackage that will translate it to nix srcs. I we want to do it that way, we need to implement https://golang.org/cmd/go/#hdr-Remote_import_paths in nix that are now implemented in Go and used by go2nix https://github.com/golang/go/blob/7bc40ffb05d8813bf9b41a331b45d37216f9e747/src/cmd/go/vcs.go#L817.
The most hard part of it will be:

If the import path is not a known code hosting site and also lacks a version control qualifier, the go tool attempts to fetch the import over https/http and looks for a tag in the document's HTML .

So build of a Go package will have to call a site and parse HTML to know code location and make proper src.
Another thing is that there can be package fork that need to be used instead of its origin, like optional repo in https://glide.readthedocs.io/en/latest/glide.yaml/

The most important question for me here is if we want to replace deps.json/libs.json with per binary deps.nix without any helpers because it's just better to have deps in one enclosed nix file or we can withdraw this PR and come later with a more complete solution implemented in nix?

@kamilchm kamilchm changed the title [WIP] buildGoPackage: move goDeps from JSON to nix buildGoPackage: move goDeps from JSON to nix Aug 1, 2016
@garbas
Copy link
Member

garbas commented Aug 3, 2016

could go2nix also use fix' like it is in haskell and pypi2nix? that would mean go2nix would initially generate 2 package. deps.nix and deps_overrides.nix. on second run it would only regenerate deps.nix. so if you want to override somethin gin deps.nix you would do it via deps_overrides.nix.

@kamilchm
Copy link
Member Author

kamilchm commented Aug 3, 2016

It's doable. Could you provide some examples in nixpkgs that will describe the exact use cases?
Besides that, even if I implement it now there are no packages in nixpkgs that will use it right now and I wonder if it's worth to add tens of empty files at the moment?
I opt for doing it when we find a real need of it in any new Go program introduced in the future PRs. Then I'll do it with a working example in real nixpkgs.

go2nix should reflect state of nixpkgs and features should flow like:
nixpkgs needs/use something -> manually implemented and merged package in nixpkgs -> go2nix generates new form of derivation

@kamilchm
Copy link
Member Author

kamilchm commented Aug 8, 2016

I'll be out of the github from the next week till september. So if the maintainers have any comments that can move this forward to the master I can try to get it merged if I manage to do it this week.

@garbas
Copy link
Member

garbas commented Aug 9, 2016

👍 i'm for merging this. improvements can come later.

@zimbatm
Copy link
Member

zimbatm commented Aug 10, 2016

@kamilchm sorry for the late reply. I think we should try to avoid changing the deps format too much. Having the tooling produce the wrong format is going to be confusing otherwise.

I think we all agree that the libs.json is not necessary, so that could go into it's own PR.

Regarding the deps format, I think it would be best to keep in NIXON format (aka: only the data bits). How hard would it be to output the exact same JSON data structure but in Nix instead? (and maybe add a version format key so we can evolve the schema)

@kamilchm
Copy link
Member Author

@zimbatm do you have an example where and how can I place a version?

@kamilchm
Copy link
Member Author

I'll try to get rid of the libs.json from master in a separate PR for tomorrow.

@zimbatm if I convert current deps.json to nix it will look like this:

[
  {
    goPackagePath = "bitbucket.org/ww/goautoneg";
    fetch = {
      type = "hg";
      url = "bitbucket.org/ww/goautoneg";
      rev = "75cd24fc2f2c2a2088577d12123ddee5f54e0675";
      sha256 = "19khhn5xhqv1yp7d6k987gh5w5rhrjnp4p0c6fyrd8z6lzz5h9qi";
    };
  }
  {
    goPackagePath = "golang.org/x/crypto";
    fetch = {
      type = "git";
      url = "https://go.googlesource.com/crypto"
      rev = "1f22c0103821b9390939b6776727195525381532";
      sha256 = "1acy12f396sr3lrnbcnym5q72qnlign5bagving41qijzjnc219m";
    };
  }
]

I also showed more pure version before in this comment that will require a lot more stuff to be done as nix utility function.
So the main question is: do we want to have an advanced godeps like function in nix at the end? If so, is it worth do convert current deps.json to simple NIXON as a intermediate step?

@avnik
Copy link
Contributor

avnik commented Aug 12, 2016

I am agreed, that NIXON will be much better, and cleaner.
But also this PR block any work on adding go packages as well (I'd like to add drone.io CLI)

@kamilchm
Copy link
Member Author

kamilchm commented Aug 12, 2016

What we need is someone from maintainers who can say what would be a desired state and what steps needs to be done to get there. Someone who can then merge subsequent PRs into master if we do the work.

@avnik as for the adding new packages now, I think I can migrate any package and I'll do this in every following PR.

BTW. I'll be out from tomorrow till september.

@zimbatm
Copy link
Member

zimbatm commented Aug 12, 2016

@kamilchm sorry for the delay.

So the main question is: do we want to have an advanced godeps like function in nix at the end?
In my view go2nix should do the heavy lifting and resolve aliases to keep the nix code simple. It can take dependencies from the GOPATH or from existing manifest like Godeps.json or vendor/vendor.json.

If you can just convert the existing JSON data structures to NIXON and replace the importJSON with importit's probably enough. If you want forward and backward-compatibility I would also wrap that NIXON with another layer so that a schema version can be specified. Eg:

{
  version = 1;
  srcs = [ # the usual
  ];
}

@kamilchm
Copy link
Member Author

kamilchm commented Sep 2, 2016

If anyone missed it, there is a new effort and committee for Go package management https://groups.google.com/d/msg/golang-nuts/bErS3fDwIgU/DRSUfnS1FQAJ
I'll keep eye on it :)

@edolstra
Copy link
Member

edolstra commented Sep 7, 2016

Any progress on this? It would be good to get this merged before 16.09 so that we don't have to support that JSON craziness forever.

@edolstra
Copy link
Member

edolstra commented Sep 7, 2016

One concern here is the use of fetchgit, which should really never be used in Nixpkgs, because it's not deterministic. Any package that depends on it will fail to build randomly at some point in the future. OTOH, it looks like the Go infrastructure already uses fetchgit so this PR doesn't really make the situation worse.

@kamilchm
Copy link
Member Author

kamilchm commented Sep 7, 2016

I'll have time on friday to create new PR with JSON converted to NIXON (#17254 (comment)) with corresponding go2nix version.
Is it ok for 16.09?

kamilchm added a commit to kamilchm/go2nix that referenced this pull request Sep 10, 2016
@kamilchm
Copy link
Member Author

PR with all deps in pure NIXON form -> #18487
I'm closing this one.

@kamilchm kamilchm closed this Sep 10, 2016
kamilchm added a commit to kamilchm/nixpkgs that referenced this pull request Sep 15, 2016
zimbatm pushed a commit that referenced this pull request Sep 15, 2016
#17254 (comment)

* update docs to describe `deps.nix`
* include goDeps in nix-shell GOPATH
* NixOS 16.09 rel notes about replacing goPackages
zimbatm pushed a commit that referenced this pull request Sep 15, 2016
#17254 (comment)

* update docs to describe `deps.nix`
* include goDeps in nix-shell GOPATH
* NixOS 16.09 rel notes about replacing goPackages

(cherry picked from commit 914e0e5)

Conflicts:
  pkgs/servers/etcd/deps.json
globin pushed a commit to mayflower/nixpkgs that referenced this pull request Sep 16, 2016
NixOS#17254 (comment)

* update docs to describe `deps.nix`
* include goDeps in nix-shell GOPATH
* NixOS 16.09 rel notes about replacing goPackages
qknight pushed a commit to nixcloud/go2nix that referenced this pull request Oct 30, 2016
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
NixOS#17254 (comment)

* update docs to describe `deps.nix`
* include goDeps in nix-shell GOPATH
* NixOS 16.09 rel notes about replacing goPackages

(cherry picked from commit 914e0e5)

Conflicts:
  pkgs/servers/etcd/deps.json
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.

8 participants