check meta, treewide #32365

vcunat · 2017-12-05T23:23:42Z

Plan:

wait a bit for feedback (some parts might be disputable)
backport to 17.09
integrate into borg checks

?: add into the tarball job, simple pre-push hook for direct pushers, more meta checks, ...

vcunat · 2017-12-05T23:27:59Z

/cc @grahamc.

vcunat · 2017-12-05T23:33:19Z

and /cc @copumpkin who started checkMeta.

vcunat · 2017-12-05T23:39:56Z

For now I used nix-env -f checkMeta.nix -qa --no-name --out-path --show-trace >/dev/null with some code adapted from rebuild-amount.sh.

7c6f434c · 2017-12-05T23:47:22Z

For some weird GitHub bug I cannot leave a line comment; wouldn't it be more consistent to also add the current platform to the message for Parallels tools?

I guess perfect consistency w.r.t. system/platform and colon/no-colon is too annoying to ensure across all the changes (for little gain).

In general looks good, but not sure if GitHub hides some part of diff because it is too large…

grahamc · 2017-12-05T23:51:20Z

pkgs/applications/taxes/aangifte-2006/default.nix

@@ -5,15 +5,15 @@ stdenv.mkDerivation {
  builder = ./builder.sh;

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2006_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2006_linux.tar.gz;


this one isn't right

Oh, aangifte-* were the only not done fully manually and they got wrong without me noticing :-/

grahamc

looks great other than these very minor issues :)

grahamc · 2017-12-05T23:51:42Z

pkgs/applications/taxes/aangifte-2007/default.nix

@@ -5,7 +5,7 @@ stdenv.mkDerivation {
  builder = ./builder.sh;

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2007_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2007_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:51:55Z

pkgs/applications/taxes/aangifte-2008/default.nix

@@ -6,7 +6,7 @@ stdenv.mkDerivation {
  builder = ./builder.sh;

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2008_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2008_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:52:04Z

pkgs/applications/taxes/aangifte-2009/default.nix

@@ -4,7 +4,7 @@ stdenv.mkDerivation {
  name = "aangifte2009-1";

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2009_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2009_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:52:10Z

pkgs/applications/taxes/aangifte-2010/default.nix

@@ -4,7 +4,7 @@ stdenv.mkDerivation {
  name = "aangifte2010-1";

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2010_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2010_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:52:29Z

pkgs/applications/taxes/aangifte-2011/default.nix

@@ -4,7 +4,7 @@ stdenv.mkDerivation {
  name = "aangifte2011-1";

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2011_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2011_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:52:36Z

pkgs/applications/taxes/aangifte-2012/default.nix

@@ -4,7 +4,7 @@ stdenv.mkDerivation {
  name = "aangifte2012-1";

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2012_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2012_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:52:44Z

pkgs/applications/taxes/aangifte-2013-wa/default.nix

@@ -4,7 +4,7 @@ stdenv.mkDerivation {
  name = "aangifte2013-wa";

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/wa2013_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/wa2013_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:52:52Z

pkgs/applications/taxes/aangifte-2013/default.nix

@@ -4,7 +4,7 @@ stdenv.mkDerivation {
  name = "aangifte2013-1";

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2013_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2013_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:53:01Z

pkgs/applications/taxes/aangifte-2014-wa/default.nix

@@ -4,7 +4,7 @@ stdenv.mkDerivation {
  name = "aangifte2014-wa";

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/wa2014_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/wa2014_linux.tar.gz;


url / homepage confusion

grahamc · 2017-12-05T23:53:08Z

pkgs/applications/taxes/aangifte-2014/default.nix

@@ -4,7 +4,7 @@ stdenv.mkDerivation {
  name = "aangifte2014-1";

  src = fetchurl {
-    url = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2014_linux.tar.gz;
+    homepage = http://download.belastingdienst.nl/belastingdienst/apps/linux/ib2014_linux.tar.gz;


url / homepage confusion

grahamc

I think I don't fully grok the implications around abort vs. throw but I strongly approve of this!

vcunat · 2017-12-05T23:58:21Z

We could add something like stdenv.throwSystem or frameworkize this in some other way :-)

grahamc · 2017-12-06T01:10:22Z

pkgs/stdenv/generic/check-meta.nix

@@ -126,7 +126,7 @@ let
      '';

  throwEvalHelp = { reason , errormsg ? "" }:
-    throw (''
+    (if reason != "unknown-meta" then throw else (x : builtins.trace x true)) (''


I'd be tempted to make this configurable so we can highlight or ignore certain conditions depending on what we're testing for:

updated

diff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix index 03a85633ed6..6e97ca7534d 100644 --- a/pkgs/stdenv/generic/check-meta.nix +++ b/pkgs/stdenv/generic/check-meta.nix @@ -125,11 +125,17 @@ let ''; - throwEvalHelp = { reason , errormsg ? "" }: - throw ('' - Package ‘${attrs.name or "«name-missing»"}’ in ${pos_str} ${errormsg}, refusing to evaluate. + handleEvalIssue = { reason , errormsg ? "" }: + let + msg = '' + Package ‘${attrs.name or "«name-missing»"}’ in ${pos_str} ${errormsg}, refusing to evaluate. - '' + ((builtins.getAttr reason remediation) attrs)); + '' + (builtins.getAttr reason remediation) attrs; + + handler = if config ? "handleEvalIssue" + then config.handleEvalIssue reason + else throw; + in handler msg; metaTypes = with lib.types; rec { # These keys are documented @@ -192,7 +198,7 @@ let validityCondition = let v = checkValidity attrs; in if !v.valid - then throwEvalHelp (removeAttrs v ["valid"]) + then handleEvalIssue (removeAttrs v ["valid"]) else true; in

usage:

handleEvalIssue = reason: errormsg: if reason == "unknown-meta" then abort errormsg else builtins.trace errormsg true;

If we play our cards right, we could take evaluation aborted with the following error message: ‘Package ‘aangifte2006-1’ in /home/grahamc/projects/nixpkgs/pkgs/applications/taxes/aangifte-2006/default.nix:15 has an invalid meta attrset: parse the path an line, and leave in-line feedback on PRs..........

I'm also tempted by this.

Oh, I think this will be a good way to clean https://hydra.nixos.org/jobset/nixpkgs/trunk#tabs-errors 👍

oxij · 2017-12-06T23:52:28Z

If I were you I would squash 374eba46ee9488e242b0dae5cec163bcc8cf27bf and ba940713764d43bb3f735d4c74fbc5162e83726a (they LGTM) and cherry-pick the result to master and 17.09 as soon as possible. The leftover diff would get much smaller and users which don't really care about the frameworkization part (e.g. me) would instantly get happy.

- tracing seems annoying enough - we get errors for all packages instead of aborting on the first one - easier to differentiate from unwanted packages (broken, unfree, etc.)

They aren't meant to be critical (uncatchable) errors. Tested with nix-env + checkMeta: [ "x86_64-linux" "i686-linux" "x86_64-darwin" "aarch64-linux" ]

vcunat added 0.kind: enhancement 8.has: clean-up 9.needs: port to stable labels Dec 5, 2017

vcunat requested review from edolstra and FRidh as code owners December 5, 2017 23:23

vcunat force-pushed the p/check-meta branch from 7cbd6d1 to 6c15cff Compare December 5, 2017 23:27

grahamc reviewed Dec 5, 2017

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Dec 5, 2017

grahamc reviewed Dec 5, 2017

View reviewed changes

grahamc approved these changes Dec 5, 2017

View reviewed changes

grahamc mentioned this pull request Dec 6, 2017

pkgs: fix some metas #32342

Merged

8 tasks

grahamc reviewed Dec 6, 2017

View reviewed changes

This was referenced Dec 7, 2017

treewide: checkMeta fixes for x86_64-linux #32410

Merged

Backport meta fixes from #32365 to 17.099 #32448

Merged

vcunat and others added 3 commits December 12, 2017 18:07

treewide platform checks: abort -> throw

3a110ea

They aren't meant to be critical (uncatchable) errors. Tested with nix-env + checkMeta: [ "x86_64-linux" "i686-linux" "x86_64-darwin" "aarch64-linux" ]

stdenv: allow specifying a eval issuee handler

f33a513

grahamc force-pushed the p/check-meta branch from ba94071 to f33a513 Compare December 12, 2017 23:10

GrahamcOfBorg added 10.rebuild-linux: 0 and removed 10.rebuild-linux: 1-10 labels Dec 12, 2017

grahamc merged commit e5629dc into NixOS:master Dec 12, 2017

grahamc mentioned this pull request Dec 13, 2017

Backporting meta fixes and from master #32620

Merged

8 tasks

vcunat deleted the p/check-meta branch December 16, 2017 09:29

samueldr removed the 9.needs: port to stable label Apr 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check meta, treewide #32365

check meta, treewide #32365

vcunat commented Dec 5, 2017 •

edited by grahamc

Loading

vcunat commented Dec 5, 2017

vcunat commented Dec 5, 2017

vcunat commented Dec 5, 2017

7c6f434c commented Dec 5, 2017

grahamc Dec 5, 2017

vcunat Dec 5, 2017

grahamc left a comment

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc Dec 5, 2017

grahamc left a comment

vcunat commented Dec 5, 2017

grahamc Dec 6, 2017 •

edited

Loading

grahamc Dec 6, 2017

vcunat Dec 6, 2017

vcunat Dec 12, 2017

oxij commented Dec 6, 2017 via email

check meta, treewide #32365

check meta, treewide #32365

Conversation

vcunat commented Dec 5, 2017 • edited by grahamc Loading

Plan:

vcunat commented Dec 5, 2017

vcunat commented Dec 5, 2017

vcunat commented Dec 5, 2017

7c6f434c commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc left a comment

Choose a reason for hiding this comment

vcunat commented Dec 5, 2017

grahamc Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxij commented Dec 6, 2017 via email

vcunat commented Dec 5, 2017 •

edited by grahamc

Loading

grahamc Dec 6, 2017 •

edited

Loading