Revert "ci: module maintainer review requests; nixos/modules: init `meta.teams`" (#497123) (e59f2c1d) · Commits · nix / nixpkgs

ci/eval/compare/default.nix

+6 −5

Original line number	Diff line number	Diff line
		@@ -123,7 +123,9 @@ let
		# - values: lists of `packagePlatformPath`s
		diffAttrs = builtins.fromJSON (builtins.readFile "${combined}/combined-diff.json");

		changedPackagePlatformAttrs = convertToPackagePlatformAttrs diffAttrs.changed;
		rebuildsPackagePlatformAttrs = convertToPackagePlatformAttrs diffAttrs.rebuilds;
		removedPackagePlatformAttrs = convertToPackagePlatformAttrs diffAttrs.removed;

		changed-paths =
		let
		@@ -160,10 +162,9 @@ let

		inherit
		(callPackage ./maintainers.nix {
		affectedAttrPaths = map (a: a.packagePath) (
		convertToPackagePlatformAttrs (diffAttrs.changed ++ diffAttrs.removed)
		);
		changedFiles = lib.importJSON touchedFilesJson;
		changedattrs = lib.attrNames (lib.groupBy (a: a.name) changedPackagePlatformAttrs);
		changedpathsjson = touchedFilesJson;
		removedattrs = lib.attrNames (lib.groupBy (a: a.name) removedPackagePlatformAttrs);
		})
		users
		teams
		@@ -180,7 +181,7 @@ runCommand "compare"
		];
		users = builtins.toJSON users;
		teams = builtins.toJSON teams;
		packages = builtins.toJSON (lib.map (lib.concatStringsSep ".") packages);
		packages = builtins.toJSON packages;
		passAsFile = [
		"users"
		"teams"

ci/eval/compare/maintainers.nix

+70 −86

Original line number	Diff line number	Diff line
		# Figure out which maintainers (users/teams) are relevant for a PR:
		# - All maintainers that can be linked directly to changedFiles
		# - Maintainers of affectedAttrPaths if a file directly related to the attribute is in changedFiles
		#
		# Files and attributes are linked in various ways:
		# - pkgs/by-name/<attr>/* is linked to pkgs.<attr>
		# - The file position of various attributes of pkgs.<attr>
		# - Explicitly specified file positions in derivations
		#
		# Test with
		# nix-instantiate --eval --strict --json test.nix -A result \| jq
		#
		# Empty list as an output means success
		{
		# Files that were changed
		# Type: ListOf (Nixpkgs-root-relative path)
		changedFiles,
		# Attributes whose value was affected by the change
		# Type: ListOf (ListOf String)
		affectedAttrPaths,

		pkgs ? import ../../.. {
		lib,
		changedattrs,
		changedpathsjson,
		removedattrs,
		}:
		let
		pkgs = import ../../.. {
		system = "x86_64-linux";
		# We should never try to ping maintainers through package aliases, this can only lead to errors.
		# One example case is, where an attribute is a throw alias, but then re-introduced in a PR.
		# This would trigger the throw. By disabling aliases, we can fallback gracefully below.
		config.allowAliases = false;
		overlays = [ ];
		},
		lib,
		}:
		let
		nixpkgsRoot = toString ../../.. + "/";
		stripNixpkgsRootFromKeys = lib.mapAttrs' (
		file: value: lib.nameValuePair (lib.removePrefix nixpkgsRoot file) value
		);
		};

		moduleMeta = (pkgs.nixos { }).config.meta;
		changedpaths = lib.importJSON changedpathsjson;

		# Currently just nixos module maintainers, but in the future we can use this for code owners too
		fileUsers = stripNixpkgsRootFromKeys moduleMeta.maintainers;
		fileTeams = stripNixpkgsRootFromKeys moduleMeta.teams;
		# Extract attributes that changed from by-name paths.
		# This allows pinging reviewers for pure refactors.
		touchedattrs = lib.pipe changedpaths [
		(lib.filter (changed: lib.hasPrefix "pkgs/by-name/" changed && changed != "pkgs/by-name/README.md"))
		(map (lib.splitString "/"))
		(map (path: lib.elemAt path 3))
		lib.unique
		];

		anyMatchingFile = filename: lib.any (lib.hasPrefix filename) changedFiles;
		anyMatchingFile = filename: lib.any (lib.hasPrefix filename) changedpaths;

		anyMatchingFiles = files: lib.any anyMatchingFile files;

		sharded = name: "${lib.substring 0 2 name}/${name}";

		attrsWithMaintainers = lib.pipe (changedattrs ++ removedattrs ++ touchedattrs) [
		# An attribute can appear in changed/removed and touched
		lib.unique
		(map (
		name:
		let
		path = lib.splitString "." name;
		# Some packages might be reported as changed on a different platform, but
		# not even have an attribute on the platform the maintainers are requested on.
		# Fallback to `null` for these to filter them out below.
		package = lib.attrByPath path null pkgs;
		in
		{
		inherit name package;
		# Adds all files in by-name to each package, no matter whether they are discoverable
		# via meta attributes below. For example, this allows pinging maintainers for
		# updates to .json files.
		# TODO: Support by-name package sets.
		filenames = lib.optional (lib.length path == 1) "pkgs/by-name/${sharded (lib.head path)}/";
		# meta.maintainers also contains all individual team members.
		# We only want to ping individuals if they're added individually as maintainers, not via teams.
		users = package.meta.nonTeamMaintainers or [ ];
		teams = package.meta.teams or [ ];
		}
		))
		# No need to match up packages without maintainers with their files.
		# This also filters out attributes where `package = null`, which is the
		# case for libintl, for example.
		(lib.filter (pkg: pkg.users != [ ] \|\| pkg.teams != [ ]))
		];

		relevantFilenames =
		drv:
		(lib.unique (
		map (pos: lib.removePrefix nixpkgsRoot pos.file) (
		map (pos: lib.removePrefix "${toString ../../..}/" pos.file) (
		lib.filter (x: x != null) [
		(drv.meta.maintainersPosition or null)
		(drv.meta.teamsPosition or null)
		@@ -71,82 +87,50 @@ let
		)
		));

		relevantAffectedAttrPaths = lib.filter (
		attrPath:
		# Some packages might be reported as changed on a different platform, but
		# not even have an attribute on the platform the maintainers are requested on.
		# Fallback to `null` for these to filter them out
		let
		package = lib.attrByPath attrPath null pkgs;
		in
		package != null && anyMatchingFiles (relevantFilenames package)
		) affectedAttrPaths;
		attrsWithFilenames = map (
		pkg: pkg // { filenames = pkg.filenames ++ relevantFilenames pkg.package; }
		) attrsWithMaintainers;

		# Extract attributes that changed from by-name paths.
		# This allows pinging reviewers for pure refactors.
		changedByNameAttrPaths = lib.pipe changedFiles [
		(lib.filter (changed: lib.hasPrefix "pkgs/by-name/" changed))
		(map (lib.splitString "/"))
		# Filters out e.g. pkgs/by-name/README.md
		(lib.filter (path: lib.length path > 3))
		(map (path: lib.elemAt path 3))
		(map lib.singleton)
		];

		# An attribute can appear in affected and touched
		attrPathsToGetMaintainersFor = lib.unique (relevantAffectedAttrPaths ++ changedByNameAttrPaths);

		attrPathEntities = lib.concatMap (
		attrPath:
		let
		package = lib.getAttrFromPath attrPath pkgs;
		in
		# meta.maintainers also contains all individual team members.
		# We only want to ping individuals if they're added individually as maintainers, not via teams.
		userPings { inherit attrPath; } (package.meta.nonTeamMaintainers or [ ])
		++ lib.concatMap (teamPings { inherit attrPath; }) (package.meta.teams or [ ])
		) attrPathsToGetMaintainersFor;

		changedFileEntities = lib.concatMap (
		file:
		userPings { inherit file; } (fileUsers.${file} or [ ])
		++ lib.concatMap (teamPings { inherit file; }) (fileTeams.${file} or [ ])
		) changedFiles;
		attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames;

		userPings =
		context:
		pkg:
		map (maintainer: {
		type = "user";
		userId = maintainer.githubId;
		inherit context;
		packageName = pkg.name;
		});

		teamPings =
		context: team:
		if team ? githubId then
		pkg: team:
		if team ? github then
		[
		{
		type = "team";
		teamId = team.githubId;
		inherit context;
		packageName = pkg.name;
		}
		]
		else
		userPings context team.members;
		userPings pkg team.members;

		maintainersToPing = lib.concatMap (
		pkg: userPings pkg pkg.users ++ lib.concatMap (teamPings pkg) pkg.teams
		) attrsWithModifiedFiles;

		byType = lib.groupBy (ping: ping.type) (attrPathEntities ++ changedFileEntities);
		byType = lib.groupBy (ping: ping.type) maintainersToPing;

		byUser = lib.pipe (byType.user or [ ]) [
		(lib.groupBy (ping: toString ping.userId))
		(lib.mapAttrs (_user: lib.map (pkg: pkg.context)))
		(lib.mapAttrs (_user: lib.map (pkg: pkg.packageName)))
		];
		byTeam = lib.pipe (byType.team or [ ]) [
		(lib.groupBy (ping: toString ping.teamId))
		(lib.mapAttrs (_team: lib.map (pkg: pkg.context)))
		(lib.mapAttrs (_team: lib.map (pkg: pkg.packageName)))
		];
		in
		{
		users = byUser;
		teams = byTeam;
		packages = attrPathsToGetMaintainersFor;
		packages = lib.catAttrs "name" attrsWithModifiedFiles;
		}

ci/eval/compare/test.nix

deleted100644 → 0

+0 −228

Original line number	Diff line number	Diff line
		{
		pkgs ? import ../../.. {
		config = { };
		overlays = [ ];
		},
		lib ? pkgs.lib,
		}:
		let
		fun = import ./maintainers.nix;

		mockPkgs =
		{
		packages ? [ ],
		modules ? [ ],
		githubTeams ? true,
		}:
		lib.updateManyAttrsByPath
		(lib.imap0 (i: p: {
		path = p;
		update = _: {
		meta.maintainersPosition.file = lib.concatStringsSep "/" p;
		meta.nonTeamMaintainers = [ { githubId = i; } ];
		meta.teams =
		if githubTeams then [ { githubId = i + 100; } ] else [ { members = [ { githubId = i + 100; } ]; } ];
		};
		}) packages)
		{
		nixos =
		{ }:
		{
		config.meta.maintainers = lib.listToAttrs (
		lib.imap0 (i: m: lib.nameValuePair m [ { githubId = i; } ]) modules
		);
		config.meta.teams = lib.listToAttrs (
		lib.imap0 (
		i: m:
		lib.nameValuePair m (
		if githubTeams then [ { githubId = i + 100; } ] else [ { members = [ { githubId = i + 100; } ]; } ]
		)
		) modules
		);
		};
		};

		tests = {
		testEmpty = {
		expr = fun {
		pkgs = mockPkgs { };
		inherit lib;
		changedFiles = [ ];
		affectedAttrPaths = [ ];
		};
		expected = {
		packages = [ ];
		teams = { };
		users = { };
		};
		};
		testNonExistentAffected = {
		expr = fun {
		pkgs = mockPkgs { };
		inherit lib;
		changedFiles = [ "a" ];
		affectedAttrPaths = [ [ "b" ] ];
		};
		expected = {
		packages = [ ];
		teams = { };
		users = { };
		};
		};
		testIrrelevantAffected = {
		expr = fun {
		pkgs = mockPkgs {
		packages = [ [ "b" ] ];
		};
		inherit lib;
		changedFiles = [ "a" ];
		affectedAttrPaths = [ [ "b" ] ];
		};
		expected = {
		packages = [ ];
		teams = { };
		users = { };
		};
		};
		testRelevantAffected = {
		expr = fun {
		pkgs = mockPkgs {
		packages = [ [ "b" ] ];
		};
		inherit lib;
		# Also tests that subpaths work
		changedFiles = [ "b/c" ];
		affectedAttrPaths = [ [ "b" ] ];
		};
		expected = {
		packages = [ [ "b" ] ];
		teams."100" = [
		{ attrPath = [ "b" ]; }
		];
		users."0" = [
		{ attrPath = [ "b" ]; }
		];
		};
		};
		testRelevantAffectedNonGitHub = {
		expr = fun {
		pkgs = mockPkgs {
		packages = [ [ "b" ] ];
		githubTeams = false;
		};
		inherit lib;
		changedFiles = [ "b/c" ];
		affectedAttrPaths = [ [ "b" ] ];
		};
		expected = {
		packages = [ [ "b" ] ];
		teams = { };
		users."0" = [
		{ attrPath = [ "b" ]; }
		];
		users."100" = [
		{ attrPath = [ "b" ]; }
		];
		};
		};
		testByNameChanged = {
		expr = fun {
		pkgs = mockPkgs {
		packages = [ [ "hello" ] ];
		};
		inherit lib;
		changedFiles = [ "pkgs/by-name/he/hello/sources.json" ];
		affectedAttrPaths = [ ];
		};
		expected = {
		packages = [ [ "hello" ] ];
		teams."100" = [
		{ attrPath = [ "hello" ]; }
		];
		users."0" = [
		{ attrPath = [ "hello" ]; }
		];
		};
		};
		testByNameReadmeChanged = {
		expr = fun {
		pkgs = mockPkgs {
		packages = [ [ "hello" ] ];
		};
		inherit lib;
		changedFiles = [ "pkgs/by-name/README.md" ];
		affectedAttrPaths = [ ];
		};
		expected = {
		packages = [ ];
		teams = { };
		users = { };
		};
		};
		testNoDuplicates = {
		expr = fun {
		pkgs = mockPkgs {
		packages = [ [ "hello" ] ];
		};
		inherit lib;
		changedFiles = [
		"hello"
		"pkgs/by-name/he/hello/sources.json"
		];
		affectedAttrPaths = [ [ "hello" ] ];
		};
		expected = {
		packages = [ [ "hello" ] ];
		teams."100" = [
		{ attrPath = [ "hello" ]; }
		];
		users."0" = [
		{ attrPath = [ "hello" ]; }
		];
		};
		};
		testModuleMaintainers = {
		expr = fun {
		pkgs = mockPkgs {
		modules = [ "a" ];
		};
		inherit lib;
		changedFiles = [ "a" ];
		affectedAttrPaths = [ ];
		};
		expected = {
		packages = [ ];
		teams."100" = [
		{ file = "a"; }
		];
		users."0" = [
		{ file = "a"; }
		];
		};
		};
		testModuleMaintainersNonGithub = {
		expr = fun {
		pkgs = mockPkgs {
		modules = [ "a" ];
		githubTeams = false;
		};
		inherit lib;
		changedFiles = [ "a" ];
		affectedAttrPaths = [ ];
		};
		expected = {
		packages = [ ];
		teams = { };
		users."100" = [
		{ file = "a"; }
		];
		users."0" = [
		{ file = "a"; }
		];
		};
		};
		};
		in
		{
		result = lib.runTests tests;
		}

modules/generic/meta-maintainers.nix

+31 −27

Original line number	Diff line number	Diff line
		@@ -4,12 +4,39 @@
		let
		inherit (lib)
		mkOption
		mkOptionType
		types
		;

		# The resulting value of this type shows where all values were defined
		sourceList = types.listOf types.raw // {
		merge = loc: defs: lib.listToAttrs (lib.map ({ file, value }: lib.nameValuePair file value) defs);
		maintainer = mkOptionType {
		name = "maintainer";
		check = email: lib.elem email (lib.attrValues lib.maintainers);
		merge = loc: defs: {
		# lib.last: Perhaps this could be merged instead, if "at most once per module"
		# is a problem (see option description).
		${(lib.last defs).file} = (lib.last defs).value;
		};
		};

		listOfMaintainers = types.listOf maintainer // {
		merge =
		loc: defs:
		lib.zipAttrs (
		lib.flatten (
		lib.imap1 (
		n: def:
		lib.imap1 (
		m: def':
		maintainer.merge (loc ++ [ "[${toString n}-${toString m}]" ]) [
		{
		inherit (def) file;
		value = def';
		}
		]
		) def.value
		) defs
		)
		);
		};
		in
		{
		@@ -17,14 +44,7 @@ in
		options = {
		meta = {
		maintainers = mkOption {
		type =
		let
		allMaintainers = lib.attrValues lib.maintainers;
		in
		lib.types.addCheck sourceList (lib.all (v: lib.elem v allMaintainers))
		// {
		description = "list of lib.maintainers";
		};
		type = listOfMaintainers;
		default = [ ];
		example = lib.literalExpression "[ lib.maintainers.alice lib.maintainers.bob ]";
		description = ''
		@@ -34,22 +54,6 @@ in
		The option value is not a list of maintainers, but an attribute set that maps module file names to lists of maintainers.
		'';
		};
		teams = mkOption {
		type =
		let
		allTeams = lib.attrValues lib.teams;
		in
		lib.types.addCheck sourceList (lib.all (v: lib.elem v allTeams))
		// {
		description = "list of lib.teams";
		};
		default = [ ];
		example = lib.literalExpression "[ lib.teams.acme lib.teams.haskell ]";
		description = ''
		List of team maintainers of each module.
		This option should be defined at most once per module.
		'';
		};
		};
		};
		meta.maintainers = with lib.maintainers; [

modules/generic/meta-maintainers/test.nix

+1 −9

Original line number	Diff line number	Diff line
		@@ -14,17 +14,9 @@ let
		};
		in
		rec {
		# Inject ghost into lib.maintainers so it passes the addCheck validation
		lib = (import ../../../lib).extend (
		final: prev: {
		maintainers = prev.maintainers // {
		inherit ghost;
		};
		}
		);
		lib = import ../../../lib;

		example = lib.evalModules {
		specialArgs.lib = lib;
		modules = [
		../meta-maintainers.nix
		{

Admin message