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

grafana: 6.7.3 -> 7.0.0 #88242

Merged
merged 1 commit into from May 23, 2020
Merged

grafana: 6.7.3 -> 7.0.0 #88242

merged 1 commit into from May 23, 2020

Conversation

JJJollyjim
Copy link
Member

@JJJollyjim JJJollyjim commented May 20, 2020

This version removes PhantomJS support.

Upstream also stopped vendoring dependencies, so I switched to buildGoModule. I do not have the go packaging experience to fully understand the implications of this -- would appreciate review from someone who does.

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

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM on NixOS 20.03:

[matt@servnerr-3:~/src/nixpkgs]$ nix-build -A grafana                                                                                                                                              
these derivations will be built:                                                                                                                                                                   
  /nix/store/3w099kjh1m33zxfxmsxg8kn8mcry2px6-remove-references-to.drv                                                                                                                             
  /nix/store/jd0qyr47s5c0p1mzz2m08rphyskz7013-source.drv                                                                                                                                           
  /nix/store/gf1ck2a6pq9pbzmj1wi1l77q5ysdy6ji-grafana-7.0.0-go-modules.drv                                                                                                                         
  /nix/store/lda1cwfim1hsz0pd0wqn8c7fcig30486-grafana-7.0.0.linux-amd64.tar.gz.drv                                                                                                                 
  /nix/store/4s5js9kkhcvqnlcbl4kzai9fwkqdy85l-grafana-7.0.0.drv 

[...]

post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0
shrinking /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0/bin/slow_proxy_mac
shrinking /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0/bin/slow_proxy
shrinking /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0/bin/grafana-server
shrinking /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0/bin/grafana-cli
shrinking /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0/bin/alert_webhook_listener
strip is /nix/store/a57856fs4m8ir6vlv14h3gq3sv9aq2lb-binutils-2.31.1/bin/strip
stripping (with command strip and flags -S) in /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0/bin
patching script interpreter paths in /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0
checking for references to /build/ in /nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0...
/nix/store/53a65a2gapdyagc5vpp6k6kh99saqqy5-grafana-7.0.0

[matt@servnerr-3:~/src/nixpkgs]$ ./result/bin/grafana-server -v
Version 7.0.0 (commit: NA, branch: master)

@mdlayher
Copy link
Member

@GrahamcOfBorg test grafana

@Ma27
Copy link
Member

Ma27 commented May 20, 2020

Before merging this, I'd like to wait for a review of @WilliButz. We should discuss if we actually want to use buildGoModule here.

@mdlayher
Copy link
Member

mdlayher commented May 20, 2020

Although I am not the maintainer of this particular package, I am experienced with Go and would support moving to buildGoModule.

The Grafana repository uses Go modules (note go.mod and go.sum in the repo root) and has removed the vendor folder, so buildGoModule seems appropriate. I use it in the CoreRAD NixOS package as well in the same scenario. (Edit: just realized I haven't bumped CoreRAD yet since removing vendor a few days ago, so this data point is less useful)

@WilliButz
Copy link
Member

@JJJollyjim first of all, thanks for taking care of the update :)

My opinion on buildGoModule (same applies to buildRustPackage) has changed over time:
Though at first it seemed to me like a very convenient way to handle the language-specific build dependencies, while only having to specify a single hash to pin them all, it takes away the control Nix had over said dependencies.
I was forced a few times to update the modSha256 as well as cargoSha256 of some packages, because some not strictly pinned dependency changed.

Because the recent update to buildGoModule using -mod=vendor seems somewhat promising and because the change needed to go from the vendored dependencies to buildGoModule is quite simple, I'd be ok with giving it a try to see how stable the grafana dependencies (and all of their dependencies) are.
However, if it should come up that vendorSha256 needs to be regularly updated, I'd suggest going for the deps.nix approach and trading some more generated Nix for a long term stable build.

On the major version update:
Could you add a short entry to the release notes for the upcoming release including a reference to the upgrade instructions?

@JJJollyjim JJJollyjim force-pushed the grafana-7.0.0 branch 2 times, most recently from b85f710 to 04274b1 Compare May 21, 2020 01:17
@JJJollyjim
Copy link
Member Author

Release notes added.

Copy link
Member

@Frostman Frostman left a comment

Choose a reason for hiding this comment

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

nixosTests.grafana passed and binaries seems to be working fine.

Copy link
Member

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM again, but left a release notes nit.

nixos/doc/manual/release-notes/rl-2009.xml Outdated Show resolved Hide resolved
This version removes PhantomJS support.

Upstream also stopped vendoring dependencies, so I switched to buildGoModule.
@JJJollyjim
Copy link
Member Author

Merge conflict resolved.

@ofborg ofborg bot requested a review from Frostman May 23, 2020 00:16
Copy link
Member

@WilliButz WilliButz left a comment

Choose a reason for hiding this comment

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

upgrade of my existing installation went smoothly, will merge after borg finished 👍

@WilliButz
Copy link
Member

@GrahamcOfBorg test grafana

@WilliButz WilliButz merged commit 10b6f23 into NixOS:master May 23, 2020
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

5 participants