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

buildGoModule: disable default go module proxy #71049

Merged
merged 1 commit into from Oct 20, 2019

Conversation

marsam
Copy link
Contributor

@marsam marsam commented Oct 12, 2019

Since GO 1.13, the go command uses by default [1]
GOPROXY="https://proxy.golang.org,direct" as module proxy, which caches
the latest signed tree size and tree hash in $GOPATH[2], hence producing
a different hash every time proxy.golang.org/latest changes.

[1] https://golang.org/cmd/go/#hdr-Module_downloading_and_verification
[2] https://go.googlesource.com/proposal/+/master/design/25530-sumdb.md#command-client

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc: @kalbasit, @c00w, @rvolosatovs

Since GO 1.13, the go command caches the lookup results and tiles in
$GOPATH[1], hence making the module directory non-deterministic.

Use the `-f` flag when removing /sumdb, for compatibility with Go 1.12
because in that version does not exists that directory.

[1] https://go.googlesource.com/proposal/+/master/design/25530-sumdb.md#command-client
@@ -48,6 +48,13 @@ let

GO111MODULE = "on";

# Disable proxying to checksum database. Since Golang 1.13, go uses
# proxy.golang.org as default GOPROXY, and it will cache the latest signed
# tree size and tree hash in $GOPATH/pkg/sumdb/<sumdb-name>/latest, hence
Copy link
Member

@kalbasit kalbasit Oct 12, 2019

Choose a reason for hiding this comment

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

Have you tried simply removing the entire $GOPATH/pkg/sumdb directory instead? If we turn off GOPROXY, we might end up with a diverging result from Go 1.13 used outside of nixpkgs. Context: kubernetes/test-infra#13918

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'm going to try it; but IIUC the test-infra issue was because the jsonpatch retagged the repository

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's correct. This will eventually happen again, but for those who are using Go 1.13 to develop their project will not notice the retag because of the proxy. If we then try to package their software, it will fail the build leading to confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think is now done.

@c00w
Copy link
Contributor

c00w commented Oct 12, 2019 via email

@marsam
Copy link
Contributor Author

marsam commented Oct 12, 2019

@c00w I'm getting different hashes on #71037, and #71037

tflint, for instance:

platform sha256
locally darwin/NixOS (no sandbox) 0dx0yh6y2hrqzz8bbcsk9jb0wwiraq12labzd42hybxkwayp96sb
ofborg aarch64 13yqa076mq0nhavh3lyq4p51lygxlfmlfs18lzapr9zpakgxkzs2
ofborg x86_64 16dm6g7maclrb8hmn7bk4l5g0w7xzylx4fss550xsd5x2ql1kc3k

@marsam marsam mentioned this pull request Oct 12, 2019
10 tasks
@c00w
Copy link
Contributor

c00w commented Oct 13, 2019

Can we try deleting pkg/mod/cache/download/sumdb during the install phase? It looks like the proof chain may be non deterministic, so deleting it may fix the output tree.

@marsam marsam force-pushed the buildGoModule-disable-goproxy branch from a748903 to 69ae8a9 Compare October 14, 2019 12:12
@marsam
Copy link
Contributor Author

marsam commented Oct 14, 2019

@kalbasit, @c00w I've updated it to remove the /sumdb directory.
I guess this change will change the modSha256 for all buildGoModule packages.

@kalbasit
Copy link
Member

@GrahamcOfBorg build jx pet gcsfuse

@kalbasit
Copy link
Member

The modSha256sum has not changed for gcsfuse, jx or pet. Are you sure this is needed?

@c00w
Copy link
Contributor

c00w commented Oct 15, 2019 via email

@marsam
Copy link
Contributor Author

marsam commented Oct 16, 2019

The modSha256sum has not changed for gcsfuse, jx or pet. Are you sure this is needed?

@c00w is right, modSha256 changed but the old hash is available locally so it doesn't build it.

If you agree with this change, I can update the hash of all the derivations affected.

@marsam marsam mentioned this pull request Oct 16, 2019
10 tasks
@c00w
Copy link
Contributor

c00w commented Oct 16, 2019 via email

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/make-buildgomodule-failsafe/4425/2

@NobbZ NobbZ mentioned this pull request Oct 19, 2019
10 tasks
@andrew-d andrew-d mentioned this pull request Oct 19, 2019
10 tasks
@c00w
Copy link
Contributor

c00w commented Oct 19, 2019

I think we should merge this as is without updating the underlying hashes. Currently if a developer is trying to update a module, without this fix they're going to have to do something relatively complicated or get frustrated and give up.

Once this fix is merged we can update the go hashes in a separate PR, or allow it to naturally happen as versions + dependencies get updated. Additionally, since most hashes haven't been changed from go1.12 (when this wasn't a problem), the number of outstanding hashes should be relatively small.

@marsam marsam force-pushed the buildGoModule-disable-goproxy branch from 69ae8a9 to 589d4ff Compare October 20, 2019 01:21
@marsam marsam merged commit e18646b into NixOS:master Oct 20, 2019
@marsam
Copy link
Contributor Author

marsam commented Oct 20, 2019

yeah, it's blocking various PRs. I'm going ahead and merging this PR, and address the mismatching hashes in another PR.

@marsam marsam deleted the buildGoModule-disable-goproxy branch October 20, 2019 01:39
@xrelkd xrelkd mentioned this pull request Oct 26, 2019
10 tasks
ento added a commit to ento/nur-packages that referenced this pull request Oct 29, 2019
minijackson added a commit to minijackson/nixpkgs that referenced this pull request Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants