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

[RFC] types.listOf: actually validate the contents #168295

Closed
wants to merge 5 commits into from
Closed

[RFC] types.listOf: actually validate the contents #168295

wants to merge 5 commits into from

Conversation

K900
Copy link
Contributor

@K900 K900 commented Apr 11, 2022

Description of changes

This is a footgun. Remove the footgun. I solemnly swear to do my best to deal with the inevitable fallout.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@K900 K900 requested review from edolstra, nbp and infinisil as code owners April 11, 2022 17:22
@K900
Copy link
Contributor Author

K900 commented Apr 11, 2022

This will probably break things. Things that are broken by this probably deserve it.

@Deep-Six
Copy link
Contributor

@K900 thanks for resolving this! It was driving me mad!

@K900
Copy link
Contributor Author

K900 commented Apr 11, 2022

For context, @Deep-Six was hitting a completely inscrutable error after doing environment.systemPackages = [ pkgs.rust ], with pkgs.rust being a random attribute set of assorted Rust stuffs and not a package.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 11, 2022
@Mindavi
Copy link
Contributor

Mindavi commented Apr 11, 2022

Aww, this is all I get from nixpkgs-review:

Result of nixpkgs-review pr 168295 run on x86_64-linux 1

1 package blacklisted:
  • nixos-install-tools

I think it'll only be in the modules, which probably requires running (some) nixos tests.

Anyway, +1 from me, seems like a good idea to check this.

K900 added 2 commits April 11, 2022 21:39
This actually works if the value is an attrset.
Just to fix things quicker for now
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Apr 11, 2022
@K900
Copy link
Contributor Author

K900 commented Apr 11, 2022

The comment saying check-meta should be enabled "soon" is from 2017... Right now I've enabled it just to see more things fail (or not?), but maybe it should stay that way?

CC @copumpkin who wrote the original comment all those years ago.

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Apr 11, 2022
@K900
Copy link
Contributor Author

K900 commented Apr 11, 2022

OK, so it turns out the validation does work correctly here, the problem is with toPretty specifically exploding when passed pkgs.rust for ... whatever reason?

@K900 K900 closed this Apr 11, 2022
@Artturin Artturin requested a review from roberth April 11, 2022 20:31
@roberth
Copy link
Member

roberth commented Apr 11, 2022

toPretty probably fails miserably on data structures that involve repetition and recursive references. Maybe that's what happened?

In error messages, we shouldn't enter more than two layers of attrsets and/or lists, to avoid infinite recursions and to avoid some unnecessarily large messages.

@K900
Copy link
Contributor Author

K900 commented Apr 11, 2022

Yes, that is what happened. I've submitted a fix (ish) for toPretty in #168327

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants