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

Add regenGoMod option #87855

Closed
wants to merge 6 commits into from
Closed

Add regenGoMod option #87855

wants to merge 6 commits into from

Conversation

@c00w
Copy link
Contributor

c00w commented May 15, 2020

Motivation for this change

A number of go modules need go mod tidy run. Due to this a number of derivations are carrying patches. This removes the patches and just teaches nix how to do this.

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.
@mweinelt

This comment was marked as outdated.

@c00w
Copy link
Contributor Author

c00w commented May 15, 2020

@mweinelt - you need to update vendorSha256 :)

@mweinelt
Copy link
Member

mweinelt commented May 15, 2020

Spot on! I tip my hat to you, sir!

@Gaelan
Copy link
Contributor

Gaelan commented May 15, 2020

All builds successfully on Darwin as well (except for firectl, which was unsupported anyway).

@mweinelt mweinelt mentioned this pull request May 15, 2020
4 of 10 tasks complete
@bhipple
Copy link
Contributor

bhipple commented May 15, 2020

Result of nixpkgs-review pr 87855 1

235 packages built:
- _3mux
- act
- aerc
- age
- amass
- antibody
- archiver
- argo
- argocd
- atlantis
- aws-vault
- awsweeper
- azure-storage-azcopy
- bat-extras.prettybat
- bazel-gazelle
- bazelisk
- berglas
- bettercap
- blockbook
- browserpass
- buildah
- caddy
- caddy2
- cassowary
- certigo
- cheat
- chezmoi
- circleci-cli
- clair
- clash
- clipman
- cloudflared
- cni-plugins
- conftest
- consul
- coredns
- corerad
- cri-o
- cri-tools
- croc
- ctop
- cue
- curlie
- dbmate
- dive
- dnsproxy
- do-agent
- docker-machine-kvm2
- docui
- documize-community
- dolt
- drone
- drone-cli
- echoip
- editorconfig-checker
- eksctl
- elvish
- exercism
- firectl
- fluxctl
- fly
- flyctl
- frp
- fscrypt-experimental
- fzf
- fzf-zsh
- geoipupdate
- ghq (gitAndTools.ghq)
- gitAndTools.gh
- gitAndTools.git-bug
- gitAndTools.git-subtrac
- gitAndTools.lab
- lefthook (gitAndTools.lefthook)
- gjo
- glow
- gmailctl
- go-ethereum
- go-jsonnet
- go-license-detector
- go-protobuf
- go-tools
- go2nix
- gobetween
- gobuster
- godef
- gofumpt
- gogetdoc
- golangci-lint
- gomatrix
- gomodifytags
- gomuks
- gopass
- gopkgs
- gopls
- gortr
- gotestsum
- gotify-cli
- gotify-server
- gotools
- gotop
- grpcui
- gvisor-containerd-shim
- hasmail
- hasura-cli
- hcloud
- helmfile
- helmsman
- hetzner-kube
- hey
- hugo
- hydroxide
- iamy
- imgproxy
- influxdb
- ipfs
- ipfs-cluster
- ipfs-migrator
- joker
- jump
- jx
- k9s
- kcli
- kepubify
- kind
- kube3d
- kubeprompt
- kubernetes-helm
- kubeseal
- kubeval
- kustomize
- lego
- lf
- libgen-cli
- linkerd
- lnd
- mage
- magnetico
- matterbridge
- mautrix-whatsapp
- minify
- minikube
- minio
- minio-client
- mkcert
- mod
- mpd-mpris
- mtail
- mutagen
- mynewt-newt
- nebula
- neo-cowsay
- netdata
- nfpm
- nix-prefetch-docker
- node-problem-detector
- obfs4
- onionshare
- onionshare-gui
- packet-cli
- packr
- pdfcpu
- pet
- pg_flame
- pgcenter
- pgmetrics
- pistol
- pixiecore
- podman
- podman-compose
- podman-unwrapped
- powerline-go
- prometheus-dnsmasq-exporter
- prometheus-mikrotik-exporter
- prometheus-varnish-exporter
- proto-contrib
- protoc-gen-doc
- protolock
- prototool
- prow
- syncthing-gtk (python27Packages.syncthing-gtk)
- qbec
- qsyncthingtray
- reftools
- renderizer
- reviewdog
- richgo
- run
- saml2aws
- sampler
- sensu-go-agent
- sensu-go-backend
- sensu-go-cli
- shadowfox
- shfmt
- shiori
- skopeo
- sops
- sourcehut.buildsrht
- sourcehut.gitsrht
- sourcehut.hgsrht
- styx
- syncthing
- syncthing-cli
- syncthing-discovery
- syncthing-relay
- tailscale
- telegraf
- tendermint
- terminal-parrot
- termshark
- terracognita
- terraform-full
- terraform-providers.elasticsearch
- terraform-providers.lxd
- terraform-providers.vpsadmin
- terraform_0_11-full
- terragrunt
- tflint
- thanos
- tinygo
- todoist
- traefik
- up
- v2ray
- vimPlugins.YouCompleteMe
- vimPlugins.vim-go
- wal-g
- websocketd
- wtf
- xmloscopy
- ycmd
- yggdrasil
- yq-go
- zoxide
- zsh-history
@@ -20,6 +20,8 @@
, vendorSha256 ? null
# Whether to delete the vendor folder supplied with the source.
, deleteVendor ? false
# Lots of packages don't have a good go.mod file, this regenerates it and uses it for downloading & building.
, regenGoMod ? false

This comment has been minimized.

Copy link
@flokli

flokli May 18, 2020

Contributor

regenGoMod needs to be documented in doc/languages-frameworks/go.xml.

This comment has been minimized.

Copy link
@Mic92

Mic92 May 18, 2020

Contributor

Is there any harm in running this by default?

This comment has been minimized.

Copy link
@maralorn

maralorn May 19, 2020

Contributor

It might obfuscate the fact, that the go.mod should be reported to upstream. On the other hand this put‘s an additional burden on to nixpkgs packagers. So I opt for making it the default.

What I don‘t know is if that might trigger vendor hashes to change on modules that use the module. Someone with more insight into go could perhaps tell, if it is possible to have two different valid go.mod files.

This comment has been minimized.

Copy link
@Mic92

Mic92 May 19, 2020

Contributor

If this is a potential source for non-reproducibility we should instead of regenerating it just log a diff file that users should include as the patch. Otherwise we are just running danger of breaking hash sums.

This comment has been minimized.

Copy link
@c00w

c00w May 21, 2020

Author Contributor

Someone with more insight into go could perhaps tell, if it is possible to have two different valid go.mod files.

I think, in general, as long as the contents of vendor contain what the code needs to build, and what's marked as explicit in vendor/modulex.txt matches what's marked as explicit in go.mod - I believe there are an infinite number of potential valid go.mod files for a given set of go code (since you can always add a go module).

What I don‘t know is if that might trigger vendor hashes to change on modules that use the module

I think there are two interpretations of this question - would we need to regenerate all the hashes? (yes). Would this cause some sort of cascading effect on go code that imports other go code? (no - we don't have go code import other nix packages).

Is there any harm in running this by default?

It sounds safe, but there is actually a massive problem. Not all go code can run go mod tidy. I think given that only 5 packages need this fix and the rest build either way, I'd prefer to not spend a lot of time updating all the hashes and flipping the go around.

If this is a potential source for non-reproducibility we should instead of regenerating it just log a diff file that users should include as the patch.

This is not a source of non reproducibility - go mod tidy should always produce the same output and is deterministic based on the source code.

doc/languages-frameworks/go.xml

I really wish nix used autogenerated documentation from source (or maybe markdown). Either way, updated.

This comment has been minimized.

Copy link
@Mic92

Mic92 May 23, 2020

Contributor

I don't understand how it can be reproducible when it only read the source code. What happens if a referenced package is updated after we added the checksum? Would it not than jump to a newer version and therefore break the old checksum?

This comment has been minimized.

Copy link
@c00w

c00w May 24, 2020

Author Contributor

Yeah - if go mod tidy is adding packages, this will cause regeneration to break. I think patches is probably the better solution since it's sticky. Hopefully the conformance of the go ecosystem will improve and we'll stop seeing horribly broken go.mod files.

@c00w
Copy link
Contributor Author

c00w commented May 21, 2020

@Mic92 - Should be updated :)

@c00w c00w force-pushed the c00w:regenGoMod branch from 52c2934 to 9feb4b2 May 21, 2020
@@ -47,6 +49,7 @@ let
removeExpr = refs: ''remove-references-to ${lib.concatMapStrings (ref: " -t ${ref}") refs}'';

deleteFlag = if deleteVendor then "true" else "false";
regenFlag = if regenGoMod then "true" else "false";

This comment has been minimized.

Copy link
@flokli

flokli May 21, 2020

Contributor

You can use boolToString regenGoMod instead of writing this manually.

@c00w c00w closed this May 24, 2020
@c00w
Copy link
Contributor Author

c00w commented May 31, 2020

Someone poked me on IRC about why this wasn't reproducible, so I'm appending a little more information.

Consider two cases
in both cases with have a go program which has an import referencing
example.com/foo

currently example.com/foo is set to V1.0

if we run regenGoMod then the go.mod file with include
example.com/foo v1.0

if example.com/foo is updated to v1.1 and someone builds the same package, they'll get
example.com/foo v1.1 in the go.mod file.

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

7 participants
You can’t perform that action at this time.