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

Modify buildGoModules to use -mod=vendor instead of the go package cache. #86376

Merged
merged 6 commits into from May 14, 2020

Conversation

@c00w
Copy link
Contributor

c00w commented Apr 30, 2020

Motivation for this change

A number of users sound concerned about ensuring we have readable source. Additionally, we have to do a hack to the pkg cache to make it reproducible, where as vendor is by design reproducible.

This is the default behaviour of go 1.14 for packages with a go version of 1.14.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Member

zimbatm left a comment

👍 good idea.

Do you mind also handling the case where the vendor/ folder already exists in the source? We have a number of upstream sources that already include that folder. At the moment those need to use buildGoPackage with all the complicated source relocating business.

I think goModSha256 = null should mean that the vendor/ folder already exists and skip the go-modules generation entirely. And change goModSha256 = "impure" for the heretic case where the go-modules is built as a input derivation instead of fixed derivation.

pkgs/development/go-modules/generic/default.nix Outdated Show resolved Hide resolved
pkgs/development/go-modules/generic/default.nix Outdated Show resolved Hide resolved
pkgs/development/go-modules/generic/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/v2ray/generic.nix Show resolved Hide resolved
@zimbatm
Copy link
Member

zimbatm commented Apr 30, 2020

Some related issues to consider:

  • #84826
  • #86349
  • I also remember somewhere somebody wanted to add a deps.nix equivalent so that all the sources could be declaratively specified instead of having them calculated with a fixed-output.

Sorry for expanding the scope of the PR. I think there is a potential to handle all these scenarios once and for all. And I am willing to put my time into making this work.

@Mic92
Copy link
Contributor

Mic92 commented Apr 30, 2020

I also remember somewhere somebody wanted to add a deps.nix equivalent so that all the sources could be declaratively specified instead of having them calculated with a fixed-output.

That person was me. I am working on it. However the PR here should not block this. This feature can be added later.

@zimbatm

This comment was marked as off-topic.

@c00w c00w force-pushed the c00w:vendor_mod branch from 3cc5654 to a6fdb68 May 1, 2020
@c00w c00w force-pushed the c00w:vendor_mod branch 3 times, most recently from fe08973 to 79fff38 May 1, 2020
@c00w
Copy link
Contributor Author

c00w commented May 1, 2020

Redoing deps is another PR - this is just changing the storage format.

@c00w c00w force-pushed the c00w:vendor_mod branch from 79fff38 to a09e229 May 1, 2020
@c00w
Copy link
Contributor Author

c00w commented May 1, 2020

vendorSha256=null means use the built in vendor folder. If it's not null we won't overwrite that folder if it exists and instead throw an error. If the upstream source is bad, we'll need to drop that folder when packaging in an intermediate derivation between og source and input src.

@c00w c00w force-pushed the c00w:vendor_mod branch 2 times, most recently from 44f18d7 to 37d3710 May 1, 2020
@c00w c00w force-pushed the c00w:vendor_mod branch 3 times, most recently from be4f03d to 017e8b8 May 1, 2020
flokli added a commit that referenced this pull request May 14, 2020
This broke in #86376

Also, fix some stray trailing whitespaces
@flokli
Copy link
Contributor

flokli commented May 14, 2020

I fixed the broken manual in 6f4f37d.

@emilazy as this PR has already been merged, it'd probably need some fixup PR.

dashmoho pushed a commit to dashmoho/nixpkgs that referenced this pull request May 14, 2020
This broke in NixOS#86376

Also, fix some stray trailing whitespaces
@zowoq
Copy link
Contributor

zowoq commented May 14, 2020

Also, as a tiny nitpick, this PR removed the (Unix-conventional) trailing newline from various files.

We have an .editorconfig, what do we think about enforcing it in PRs via CI?

@Mic92
Copy link
Contributor

Mic92 commented May 14, 2020

Maybe this could be done via an github action?

@ajs124
Copy link
Member

ajs124 commented May 14, 2020

maybe this discussion belongs to a different issue ^^

but yes, considering some of the weird formatting I've seen around nixpkgs, something like this does sound reasonable.

@c00w
Copy link
Contributor Author

c00w commented May 14, 2020

@ajs124 Any chance you can add some automation? Both the github checks + nix-review pr didn't notice this. If you give me a specific way to check I can probably add them in to nix-review, but I honestly have no idea how to build the manual.

@c00w c00w deleted the c00w:vendor_mod branch May 15, 2020
@ajs124
Copy link
Member

ajs124 commented May 15, 2020

You can build the manual through release.nix, e.g. nix-build nixos/release.nix -A manual.x86_64-linux.

Alternatively, to check more things than just the manual, you can also build a "dummy" nixos system with nix-build nixos/release.nix -A dummy. This builds a basically empty nixos system, but that way, you can make sure you're not breaking basic nixos evaluation.

@zowoq
Copy link
Contributor

zowoq commented May 15, 2020

I've opened a draft where we can continue the discussion. #87853

@nh2
Copy link
Contributor

nh2 commented May 20, 2020

Can anybody explain to me what this change does? The added docs say

The go-modules builder now uses vendorSha256 instead of modSha256

but I don't understand what the effective difference. When reading the changelog, it just looks like "we renamed something but don't say why". Same with

A number of users sound concerned about ensuring we have readable source

That somehow suggests that modSha256 did not have "readable source", but I don't understand what that even means.

Maybe an example of the shortcomings of modSha256 would be useful?

@Mic92 Mic92 mentioned this pull request May 20, 2020
0 of 10 tasks complete
@Mic92
Copy link
Contributor

Mic92 commented May 20, 2020

Can anybody explain to me what this change does?

see #88258

@doronbehar
Copy link
Contributor

doronbehar commented May 20, 2020

So this doesn't address NixOS/nix#2270 right?

@Mic92
Copy link
Contributor

Mic92 commented May 20, 2020

Not yet. However it is now easier with a vendor directory to create a generator that creates separate source derivations than it was before with the go module proxy directory.

@Ma27
Copy link
Member

Ma27 commented May 22, 2020

After seeing this in the wild for a while I'm not sure if that's the way to move forward: while the old approaches certainly had their issues, we now seem to push the task of solving messed-up dependency situations in downstream sources to our package-maintainers.

Previously I was able to work around this by generating some sort of deps.nix or by using buildGoModule to let gomod do the job of fetching the proper sources. Now it seems to me that I have to search e.g. for proper git revisions of dependencies to override those if needed which is IMHO another source of possible packaging bugs.

Also, regenerating huge patches of a vendor/ directory is at least from my PoV not really a benefit over having to maintain automagically generated Nix-expressions with dependencies.

I may have missed something, but can someone explain why this is even needed? And, am I wrong with this assumption? Or in other words, shouldn't we keep the original behavior for "problematic" packages?

@numkem
Copy link
Contributor

numkem commented May 22, 2020

I can't make this work with my repo.

expression looks like this:

{ lib, buildGoModule, modSha256 ? null, maintainers, version ? "0.0.0-dev" }:

buildGoModule rec {
  pname = "repeater";
  inherit version;

  src = ../.;

  vendorSha256 = modSha256;

  subPackages = [ "cmd/repeater" ];

  meta = with lib; {
    [...]
  };
} 

And I don't have a vendor directory at the root of my module. Without the directory I get: run 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory. With it it builds just fine. The documentation states it would run go mod vendor on it's own. Yet it doesn't look like it's doing that.

@mdlayher mdlayher mentioned this pull request May 22, 2020
4 of 10 tasks complete
@doronbehar doronbehar mentioned this pull request May 22, 2020
5 of 10 tasks complete
@Ma27
Copy link
Member

Ma27 commented May 27, 2020

@zimbatm @Mic92 @c00w would you mind taking a look at the questions I asked above? :)

@c00w
Copy link
Contributor Author

c00w commented Jun 2, 2020

@numkem Just noticed this. Can you include a full repro (i.e. with no variable parameters that only depends on nixpkgs) so I could run it? A seperate issue may also help with me responding quickly, but I can keep debugging here if it is easier for you :)

My guess is that you're setting vendorSha256=null; which says use the builtin in vendor directory. This is super useful if you have a vendor directory (or your code has no external dependencies). If that's not your intention, I'd set vendorSha256.

Another option is you copied the modSha256 hash and just set vendorSha256 to it. You need to modify the hash, so a full rebuild happens, then it will error and provide you the new hash to set.

If you're not doing either of those things, then I'll need to be able to pull the code and poke it to figure out what is going on.

@c00w
Copy link
Contributor Author

c00w commented Jun 2, 2020

@Ma27

After seeing this in the wild for a while I'm not sure if that's the way to move forward: while the old approaches certainly had their issues, we now seem to push the task of solving messed-up dependency situations in downstream sources to our package-maintainers.

The sad but glorious life of package managers :). I think we mostly are in the same state we used to be with regards to messed up dependencies. There were some regressions around go mod vendor not working will.

golang/go#38820

The other indirect question is "why do this" and the answer was that people (not me) were unhappy about using a binary format as our dependency format, since it was hard to audit and modify. It's actually a strength of the new approach that you can just use plain text patches to fix issues :)

However I really just want to get rid of buildGoPackages as much as possible, so I'm doing this to try and get #86282 merged.

Previously I was able to work around this by generating some sort of deps.nix or by using buildGoModule to let gomod do the job of fetching the proper sources. Now it seems to me that I have to search e.g. for proper git revisions of dependencies to override those if needed which is IMHO another source of possible packaging bugs.

I'm confused - buildGoModule never supported deps.nix.

If you expand on your issue a bit more I can be more prescriptive, but in general if you're having packaging issues for your own source, run go mod vendor in your directory and set vendorSha256 = null :). Then your code can build entirely from source and you don't have to deal with anything.

If you're trying to fix an issue in an upstream vendor, generally just patching go.mod solves your problems pretty well. There is a fun corner case of people using go modules to distribute c code (which is super not recommended by the go team). Making sure each c folder has a go file in it may help, alternatively, cp the c source into the directory after the fetching via a postInstall command. There are a few examples of this in the repor.

Also, regenerating huge patches of a vendor/ directory is at least from my PoV not really a benefit over having to maintain automagically generated Nix-expressions with dependencies.

Why are you needing huge patches? What are you trying to package that's exploding?

I may have missed something, but can someone explain why this is even needed? And, am I wrong with this assumption? Or in other words, shouldn't we keep the original behavior for "problematic" packages?

This happened because a lot of people were screaming at me that the go module packaging in nix was bad and we shouldn't use them. Rather than delete buildGoModule and migrate everything to buildGoPackage, I tried to address some of their complaints - one complaint was that the binary files weren't human readable, so now we have vendor directories which are human readable :). Another complaint is that go downloads don't proxy super well (since they're not just raw http fetches). That's the next complaint to fix, either by using deps.nix to generate the vendor folder, or by writing a go module proxy which can be configured via deps.nix.

TL;DR I'm just trying to get #86282 merged.

@flokli flokli mentioned this pull request Jun 19, 2020
4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.