stdenv-generic: add meta attribute checking #25304

copumpkin · 2017-04-28T22:06:49Z

This is turned off by default but I think we should fix all packages to respect it and then turn it on by default. To turn it on, just put checkMeta = true; into your nixpkgs config.

cc @shlevy @dezgeg @edolstra

Motivation for this change

I got sick of constantly seeing typos and accidental type errors in our meta tags, and people never notice them until Hydra complains. I'd probably eventually start adding a few more checks (like that platforms members are in an enum and so on) but this should get us 95% of the value.

It's turned off by default because there are tons of violations, which I'd propose fixing incrementally in separate commits. The very first failure is in binutils, which has a priority field as a string 😄

Things done

Nothing. I just tried evaluating stdenv and got some failures, so there's work to be done 😁

N.B: this is not a mass rebuild, and only affects evaluation

mention-bot · 2017-04-28T22:06:51Z

@copumpkin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @pikajude and @wmertens to be potential reviewers.

shlevy

🚢 🚢 🚢

This is turned off by default but I think we should fix all packages to respect it and then turn it on by default

copumpkin · 2017-04-29T01:55:57Z

Okay, now that this is merged, if anyone wants to join me in making small commits to fix meta attributes, please feel free 😄

copumpkin · 2017-04-29T04:27:16Z

I ran hydra-eval-jobs per @cleverca22's suggestion and I think I've fixed all remaining issues, so I switched checkMeta to true by default. Please let me know or switch to false if you object, but I'd rather sneak it in before others break more stuff.

copumpkin · 2017-04-29T04:27:32Z

The two commits were 1a4ca22 and 90b9719

edolstra · 2017-05-01T09:32:08Z

The thing is that meta is intended as an arbitrary name/value map. So it's perfectly fine to include attributes that are not "known".

edolstra · 2017-05-01T09:32:49Z

Also, what's the impact of this on Nixpkgs evaluation speed? There has been a worrying slowdown in the last few releases, probably due to the increased complexity of stdenv...

copumpkin · 2017-05-01T15:31:12Z

@edolstra my sense is that the reason you'd add stuff to meta rather than a comment is that you want it to be structured for some form of machine consumption. If you want a machine to consume it, you generally want it to have a reasonably well defined schema. So it doesn't seem too burdensome to ask people wanting to add a new meta attribute to add it to the central schema first (and it'll be easy for them to tell if they miss it because it'll fail eagerly now)

On the evaluation speed, I don't know and haven't measured yet. Where can I find evaluation times in Hydra? I guess if it slows it down excessively we can turn it off, but given the literally dozens of edits I made to nixpkgs here to fix stupid typos I found with this feature, this is solving a real problem, so I'm loath to privilege speed of Hydra evaluation (which is rare and not something user-facing, and comparatively small compared to build time) over the system silently ignoring typo'd meta tags.

7c6f434c · 2017-05-01T15:47:21Z

@copumpkin speed of evaluation is something visible via nix-env -qa; and also nix-env -i which shouldn't be used ever but oh well…

If you want a quick measurement, try measuring test-eval-release.sh, I think it should be a close proxy.

By the way, maybe the check should be done inside the tarball build, like the warning checks? Tarball failures block the channel, so they do get noticed…

(I did have to do some fixes because of this check being introduced, and I do like this check in general)

copumpkin · 2017-05-01T15:52:27Z

We could put it in the tarball I guess but feedback loop time makes a difference. People still won't ever notice if they wrote maitnainers instead of maintainers (or home/website instead of homepage), as someone else will probably notice the tarball is blocked and fix it. Of course we can use more human ways of spreading the message around like adding instructions for turning this on in the PR template but I'd rather default to a mode that doesn't silently ignore silly mistakes if we can. Anyway, I'm not near my computer now but will try to measure eval time later, if Hydra isn't already doing it somewhere.

…

On Mon, May 1, 2017 at 11:47 Michael Raskin ***@***.***> wrote: @copumpkin <https://github.com/copumpkin> speed of evaluation is something visible via nix-env -qa; and also nix-env -i which shouldn't be used ever but oh well… If you want a quick measurement, try measuring test-eval-release.sh, I think it should be a close proxy. By the way, maybe the check should be done inside the tarball build, like the warning checks? Tarball failures block the channel, so they do get noticed… (I did have to do some fixes because of this check being introduced, and I do like this check in general) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25304 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKP7_mIqhHowMeiigYamcK8ZADkyfJks5r1f6MgaJpZM4NMBSF> .

7c6f434c · 2017-05-01T16:24:55Z

can use more human ways of spreading the message around like adding instructions for turning this on in the PR template but I'd rather default

Also tagging people who broke tarball eval with an explanation of the problem seems to create some progress. Maybe a special environment variable that makes stdenv return strict or lax mkDerivation could be a good solution? If we set the lax variable value for Hydra and `nix-env -qa`, but not `test-eval-release.sh`, tarball build or `nix-build`/`nix-env -iA`, we could have the best of both worlds…

edolstra · 2017-05-01T16:26:24Z

http://hydra.nixos.org/job/nixpkgs/trunk/metrics#tabs-charts

Note the jump for e.g. nix-env.qa.values around April 29 from around 2 million to 3.5 million. Or nix-env.qa.maxresident which went from 654 MB to 732 MB.

edolstra · 2017-05-01T16:28:40Z

@7c6f434c Yeah, doing this kind of lint check in the Nixpkgs tarball job sounds good.

copumpkin · 2017-05-01T16:32:50Z

Yeah, so I guess it did go up a bit. The real question to me is what we're optimizing for. I'd obviously prefer what I'd call "developer efficiency", but I'm not the one paying for or maintaining Hydra hardware. At the same time, losing a few seconds (or <100MB of RAM) with each full-nixpkgs hydra evaluation when builds take hours or even days still seems pretty small.

I do realize that the evaluation time matters every time we do nix-env or nix-shell for users too, but even there it feels like it's small compared to downloading packages from the binary cache and tiny compared to building them.

Anyway, I can't really make the call and it's a trade-off like everything, but if I were running the project, I'd probably leave this turned on by default and accept slightly slower Hydra eval times.

7c6f434c · 2017-05-01T16:35:33Z

@copumpkin @edolstra and what about environment-variable-controlled middle ground?

copumpkin · 2017-05-01T16:38:51Z

It's already controlled by a config flag. The question is more whether we want it on by default or not.

7c6f434c · 2017-05-01T16:44:16Z

My point is that nix-build and nix-env -qa with the default config should behave differently.

On Hydra the check should be just switched off in evaluation and moved to the tarball build.

copumpkin · 2017-05-01T16:47:29Z

Ah, so we'd have nix-build always check meta, nix-env wouldn't check it, and Hydra would only check when building the tarball? I guess that isn't awful, although it does (by design) make things behave a little differently in potentially confusing ways. How do I distinguish between nix-build and nix-env (also, what would nix-shell do?) with an env var?

7c6f434c · 2017-05-01T16:53:37Z

I would set an environment variable; and nix-shell is closer to nix-build; I actually would make nix-env -i invocations of the nix-build kind and only -qa lax.

There would be some slight difference in behaviour, true, but at least in case of success the hashes would be the same, so not that bad… I doubt we can go more confusing that the current broken/override interaction.

copumpkin · 2017-05-01T17:32:22Z

Oh, you mean developers would need to set an environment variable when developing? That feels like defaulting it to off, which I didn't want.

I was hoping for something a bit like IN_NIX_SHELL that would let me distinguish between nix-build and nix-env.

My issue is just that the people who are typoing all their meta tags (often in consistently bad ways, like website, home, etc.) today probably aren't following our PRs that closely and won't know that I've added this feature. So the people who need it the most won't know that they need it.

copumpkin · 2017-05-01T17:48:09Z

I'll just turn it off by default for now, since I think I missed some broken meta in many of the python packages which don't get evaluated by Hydra. I still want to turn it on by default, but can get more community input or make an RFC or something like that if you think this is too impactful.

@edolstra where are you leaning? Did my "unstructured random data in meta is useless" point make sense? Do you think the performance issues are actually a big deal?

Trying to gauge how controversial this is, basically, to decide whether an RFC is worthwhile.

To be clear, this is what I'm proposing:

Meta is for "semi-structured" data pertaining to packages. If you want to add a new flag that is likely to affect many packages, you should probably get community feedback and submit a PR adding it to the stdenv/generic schema.
If you want freeform data, leave it in a comment or a README adjacent to the package expression file
Check the meta schema by default for developers. I don't know how to separate developers at evaluation time from users and automated builds right now, but if we can do that, I'd like that.
Make sure the meta attrset is correct in all existing packages (mostly done, except that I seem to have missed a few).

#25304 (comment)

copumpkin · 2017-05-01T17:55:44Z

I switched the default to false again (for now, I hope) in f3a05a0

7c6f434c · 2017-05-01T19:00:57Z

No-no, I meant that nix* tools would set the variable that distinguishes mass operations from narrowly targeted ones. I would hope that adding a new package includes nix-build, which is a targeted operation and would do the strict check.

7c6f434c · 2017-05-01T20:07:42Z

Distinguishing mass operations and targeted operations would be also useful for deprecation warnings.

For example, @oxij wonders is there a way to display notices that there are two different torbrowser packages (from source and binary) now.

oxij · 2017-05-01T22:06:15Z

For example, @oxij wonders is there a way to display notices that there are two different `torbrowser` packages (from source and binary) now.

While my wishes are related, its not exactly the same use case, AFAICS. E.g. I don't want users of `tor-browser-bundle-bin` to see the warning, only those who use the old `torbrowser` attr. On a second thought, though, its probably possible to do something like ~~~~ torbrowser = withWarning "..." tor-browser-bundle-bin; ~~~~ in `all-packages.nix` with `withWarning` generating new derivation with a warning in meta. Then I would even vote for evaluating such warning in `nix-env -i`. On a related note. Having a way to catch `assert` failures in derivations would be nice too. I want to fuzz boolean package options and check that packages build with all possible combinations of them, directly in nix, but nix has no `catch`. Rewriting assertions to be a list in `meta` that gets inspected only on derivation build attempt would work for that use case too.

8573 · 2017-05-08T06:12:22Z

pkgs/stdenv/generic/default.nix

+        branch = str;
+        homepage = str;
+        downloadPage = str;
+        license = either (listOf lib.types.attrs) lib.types.attrs;


The current documentation says that using strings for license is "valid", though "frowned upon". I suppose the manual should be updated?

8573 · 2017-05-08T06:15:00Z

pkgs/stdenv/generic/default.nix

+
+        # Weirder stuff that doesn't appear in the documentation?
+        version = str;
+        updateWalker = bool;


updateWalker seems to be documented.

The invalid meta.outputsToInstall has been blocking channel updates. https://mailman.science.uu.nl/pipermail/nix-dev/2017-June/023991.html

- Enable metadata checking by default, see NixOS#25304 (comment) - Check metadata before any other package issues, see NixOS#191124 (comment) - Document that type checks only apply to the top level of nested values. Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

copumpkin added the 6.topic: developer experience label Apr 28, 2017

shlevy approved these changes Apr 28, 2017

View reviewed changes

copumpkin force-pushed the check-meta branch from 3569214 to 32ae4bf Compare April 28, 2017 22:11

stdenv-generic: add meta attribute checking

32ae4bf

This is turned off by default but I think we should fix all packages to respect it and then turn it on by default

copumpkin merged commit 76137a8 into NixOS:master Apr 29, 2017

ericsagnes mentioned this pull request Apr 29, 2017

stdenv-generic: add meta attributes checks #25318

Merged

7 tasks

copumpkin added a commit that referenced this pull request May 1, 2017

stdenv: disable checkMeta by default until issues resolved

f3a05a0

#25304 (comment)

aneeshusa mentioned this pull request May 2, 2017

Fix meta priority types #25427

Merged

7 tasks

8573 reviewed May 8, 2017

View reviewed changes

copumpkin referenced this pull request Jun 26, 2017

qtinstaller: fix broken meta

ce8178e

The invalid meta.outputsToInstall has been blocking channel updates. https://mailman.science.uu.nl/pipermail/nix-dev/2017-June/023991.html

dezgeg mentioned this pull request Sep 27, 2017

Policy on checks/logic in stdenv.mkDerivation #29847

Closed

piegamesde mentioned this pull request Sep 14, 2022

check-meta.nix, lib/types.nix: type check changes, more documentation #191171

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv-generic: add meta attribute checking #25304

stdenv-generic: add meta attribute checking #25304

copumpkin commented Apr 28, 2017

mention-bot commented Apr 28, 2017

shlevy left a comment

copumpkin commented Apr 29, 2017

copumpkin commented Apr 29, 2017

copumpkin commented Apr 29, 2017

edolstra commented May 1, 2017

edolstra commented May 1, 2017

copumpkin commented May 1, 2017 •

edited

7c6f434c commented May 1, 2017

copumpkin commented May 1, 2017 via email

7c6f434c commented May 1, 2017 via email

edolstra commented May 1, 2017

edolstra commented May 1, 2017

copumpkin commented May 1, 2017 •

edited

7c6f434c commented May 1, 2017

copumpkin commented May 1, 2017

7c6f434c commented May 1, 2017

copumpkin commented May 1, 2017

7c6f434c commented May 1, 2017

copumpkin commented May 1, 2017

copumpkin commented May 1, 2017 •

edited

copumpkin commented May 1, 2017

7c6f434c commented May 1, 2017

7c6f434c commented May 1, 2017

oxij commented May 1, 2017 via email

8573 May 8, 2017

8573 May 8, 2017

stdenv-generic: add meta attribute checking #25304

stdenv-generic: add meta attribute checking #25304

Conversation

copumpkin commented Apr 28, 2017

Motivation for this change

Things done

mention-bot commented Apr 28, 2017

shlevy left a comment

Choose a reason for hiding this comment

copumpkin commented Apr 29, 2017

copumpkin commented Apr 29, 2017

copumpkin commented Apr 29, 2017

edolstra commented May 1, 2017

edolstra commented May 1, 2017

copumpkin commented May 1, 2017 • edited

7c6f434c commented May 1, 2017

copumpkin commented May 1, 2017 via email

7c6f434c commented May 1, 2017 via email

edolstra commented May 1, 2017

edolstra commented May 1, 2017

copumpkin commented May 1, 2017 • edited

7c6f434c commented May 1, 2017

copumpkin commented May 1, 2017

7c6f434c commented May 1, 2017

copumpkin commented May 1, 2017

7c6f434c commented May 1, 2017

copumpkin commented May 1, 2017

copumpkin commented May 1, 2017 • edited

copumpkin commented May 1, 2017

7c6f434c commented May 1, 2017

7c6f434c commented May 1, 2017

oxij commented May 1, 2017 via email

8573 May 8, 2017

Choose a reason for hiding this comment

8573 May 8, 2017

Choose a reason for hiding this comment

copumpkin commented May 1, 2017 •

edited

copumpkin commented May 1, 2017 •

edited

copumpkin commented May 1, 2017 •

edited