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 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 has been minimized.

@c00w
Copy link
Contributor Author

c00w commented May 15, 2020

@mweinelt - you need to update vendorSha256 :)

@mweinelt
Copy link
Member

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
10 tasks
@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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Is there any harm in running this by default?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

@@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants