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

Jump to bottom

treewide get rid of go 1.12 #83253

Merged
merged 10 commits into from Apr 29, 2020
Merged

treewide get rid of go 1.12 #83253

merged 10 commits into from Apr 29, 2020

Conversation

c00w
Copy link
Contributor

@c00w c00w commented Mar 24, 2020 •
edited

Motivation for this change

Getting rid of go1.12 dependencies so we can drop go1.12. Also this accidentally got dropped out of #81159

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.

@c00w
Copy link
Contributor Author

c00w commented Mar 24, 2020

@worldofpeace Sorry - these got dropped out of #81159

@c00w c00w changed the title treewide Drop unneeded go 1.12 overrides treewide get rid of go 1.12 Mar 24, 2020
@c00w
Copy link
Contributor Author

c00w commented Mar 24, 2020

And I got excited, so this PR now completely removes go 1.12 (and almost go1.13)

vdemeester
vdemeester approved these changes Mar 24, 2020
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Nice 👏
LGTM 🙃

pkgs/applications/networking/cluster/openshift/default.nix
@@ -9,12 +9,12 @@
with lib;

let
version = "3.11.0";
version = "4.1.0";
Copy link
Member

Choose a reason for hiding this comment

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

😍

nh2
nh2 reviewed Mar 24, 2020
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

Thanks! I've put in some small requests/comments on Consul.

pkgs/servers/consul/default.nix Show resolved Hide resolved
pkgs/servers/consul/default.nix Show resolved Hide resolved
pkgs/servers/consul/default.nix Show resolved Hide resolved
nh2
nh2 approved these changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

LGTM for the consul change.

@c00w
Copy link
Contributor Author

c00w commented Apr 20, 2020

@kalbasit @worldofpeace Any chance you could review + merge?

@worldofpeace
Copy link
Contributor

@c00w Yeah a rebase is needed. (and resolve conflicts). Merge seems good to me.

@c00w
Copy link
Contributor Author

c00w commented Apr 21, 2020

Rebased - everything still passes with

nix-build -A lnd -A perkeep -A openshift -A packr -A consul -A scaleway-cli -A prometheus-varnish-exporter -A curlie -A grpcui -A ipfs -A nomad -A gobetween -A cockroachdb -A gotestsum -A magnetico -A tendermint -A cri-o default.nix

@c00w
Copy link
Contributor Author

c00w commented Apr 21, 2020

@kalbasit @worldofpeace Should be good for another round of review :)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/147

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg eval

kalbasit
kalbasit approved these changes Apr 26, 2020
View reviewed changes
Copy link
Member

@kalbasit kalbasit left a comment

Choose a reason for hiding this comment

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

LGTM. May need another rebase to fix the eval but it looks good overall. Before you merge, we should ask the bot to build all affected packages on Linux/Mac.

@c00w
Copy link
Contributor Author

c00w commented Apr 26, 2020

Thanks :) AFAIK I don't have permission to merge, or talk to the bot, so someone needs to fix it.

The eval failure is actually a interesting bug in grahamcofborg, where if you delete a file, it fails. And since by design I'm deleting to go1.12 files, it gets hit.

@worldofpeace
Copy link
Contributor

worldofpeace commented Apr 26, 2020 •
edited

The eval error is

while evaluating the attribute 'deps' of the derivation 'bazel-watcher-0.10.3' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/pkgs/build-support/build-bazel-package/default.nix:42:10:
while evaluating the attribute 'nativeBuildInputs' of the derivation 'bazel-watcher-0.10.3-deps' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/pkgs/build-support/build-bazel-package/default.nix:45:5:
while evaluating 'getOutput' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/lib/attrsets.nix:464:23, called from undefined position:
while evaluating anonymous function at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/pkgs/stdenv/generic/make-derivation.nix:133:17, called from undefined position:
while evaluating 'callPackageWith' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/lib/customisation.nix:117:35, called from /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/pkgs/top-level/all-packages.nix:8562:13:
while evaluating 'makeOverridable' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/lib/customisation.nix:67:24, called from /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/lib/customisation.nix:121:8:
getting status of '/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/hetzner-sb65-1083082-hel1-dc2/pkgs/development/compilers/go/1.12.nix': No such file or directory

Using rg I find

pkgs/top-level/all-packages.nix
8515:  go_1_12 = callPackage ../development/compilers/go/1.12.nix ({

we need to actually remove go_1_12 in all-packages.nix and add an alias in alias.nix to throw.

@c00w
Copy link
Contributor Author

c00w commented Apr 26, 2020 •
edited

Good catch - deleted missing reference in go_1_12. I don't see go_1_11 in aliases.nix, so it seems like it's unlikely to be needed? And this + bazel-watcher seems to have fixed the bot :)

@c00w c00w force-pushed the go_112 branch 3 times, most recently from 9fc8d15 to 813f7cc Compare April 26, 2020 17:26
@c00w
Copy link
Contributor Author

c00w commented Apr 26, 2020 •
edited

Rebase on top of master, since the go 1.13 removal PR got merged.

@worldofpeace
Copy link
Contributor

I don't see go_1_11 in aliases.nix, so it seems like it's unlikely to be needed?

There isn't really a likelihood of something needing an alias or a throw, it's pretty definite if something is renamed or either removed. For the end-user, having something like

buildGo112Package = throw "buildGo112Package has been removed from nixpkgs.";

is slightly more helpful than an attribute just not existing.

@worldofpeace
Copy link
Contributor

Seeing this hasn't been done regularly we could ignore that for this particular PR, but I know these help people.

@c00w
Copy link
Contributor Author

c00w commented Apr 26, 2020

@worldofpeace Added.

@c00w
Copy link
Contributor Author

c00w commented Apr 27, 2020

Updated PR with latest master due to a conflict.

c00w added 10 commits April 28, 2020 19:41
@c00w
treewide Drop unneeded go 1.12 overrides
d1c3993
@c00w
Fix packr to be go module compatible.
accfbe0 
I updated to version 2.8.0 which is the latest on master.
Then due to the 2 different sets of go modules which are used, I split
the build into two different derivations, then merged them togethor
using symlinkJoin to have the same output structure as the existing derivation.
@c00w
Remove consul dependency on go1.12
f3c7f2f 
I updated the consul version to 1.7.2 and flipped it to building using
modules.
@c00w
Remove go1.12 from perkeep.
ffef281 
Update the version to the latest unstable on master.
@c00w
Update scaleway-cli to not be pinned to go1.12
a919118 
Switched the version to 1.20
@c00w
Update prometheus-varnish-exporter to not depend on go1.12
bdbf8a1
@c00w
Update lnd to build with go1.12
bfc482e 
Updated the version
Forced only building subpackages with main to prevent panics over
multiple modules in one repo
@c00w
Remove go1.12 from openshift
1f1d992 
Had to update the version to 4.1.0 and do a bit of munging to get this
to work
@c00w
Update bazel-watcher and make it build with go 1.14
3b2cd52
@c00w
Remove go1.12 completely.
cdfab11 
These are no longer needed.
@c00w
Copy link
Contributor Author

c00w commented Apr 28, 2020

Rebased on master.

@worldofpeace worldofpeace merged commit 4007ceb into NixOS:master Apr 29, 2020
@worldofpeace
Copy link
Contributor

Thank you for your patience @c00w and your contribution ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace left review comments

@vdemeester vdemeester vdemeester approved these changes

@kalbasit kalbasit kalbasit approved these changes

@nh2 nh2 nh2 approved these changes

@NickHu NickHu Awaiting requested review from NickHu

@cstrahan cstrahan Awaiting requested review from cstrahan

@WilliButz WilliButz Awaiting requested review from WilliButz

Assignees
No one assigned
Labels
6.topic: golang 8.has: clean-up 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@c00w @worldofpeace @nixos-discourse @vdemeester @kalbasit @nh2