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

Add flag to build package passthru.tests #397

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,10 @@ attribute set:
$ nixpkgs-review pr -p nixosTests.ferm 47077
```

Many packages also specify their associated `nixosTests` in the `passthru.tests`
attribute set. Adding the `--tests` argument will run those tests for all
packages that will be rebuild.

## Ignoring ofborg evaluations

By default, nixpkgs-review will use ofborg's evaluation result if available to
Expand Down
5 changes: 5 additions & 0 deletions nixpkgs_review/cli/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@ def common_flags() -> list[CommonFlag]:
type=regex_type,
help="Regular expression that package attributes have to match (can be passed multiple times)",
),
CommonFlag(
"--tests",
action="store_true",
help="For all packages to be built, also build their `passthru.tests`",
),
CommonFlag(
"-r",
"--remote",
Expand Down
1 change: 1 addition & 0 deletions nixpkgs_review/cli/pr.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ def pr_command(args: argparse.Namespace) -> str:
build_graph=args.build_graph,
nixpkgs_config=nixpkgs_config,
extra_nixpkgs_config=args.extra_nixpkgs_config,
build_passthru_tests=args.tests,
)
contexts.append((pr, builddir.path, review.build_pr(pr)))
except NixpkgsReviewError as e:
Expand Down
8 changes: 6 additions & 2 deletions nixpkgs_review/nix.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def was_build(self) -> bool:
return self._path_verified

def is_test(self) -> bool:
return self.name.startswith("nixosTests")
return self.name.startswith("nixosTests") or ".passthru.tests." in self.name


REVIEW_SHELL: Final[str] = str(ROOT.joinpath("nix/review-shell.nix"))
Expand Down Expand Up @@ -217,6 +217,7 @@ def nix_eval(
system: str,
allow: AllowedFeatures,
nix_path: str,
include_passthru_tests: bool = False,
) -> list[Attr]:
attr_json = NamedTemporaryFile(mode="w+", delete=False)
delete = True
Expand All @@ -239,7 +240,10 @@ def nix_eval(
if allow.ifd
else "--no-allow-import-from-derivation",
"--expr",
f"(import {eval_script} {{ attr-json = {attr_json.name}; }})",
f"""(import {eval_script} {{
attr-json = {attr_json.name};
include-passthru-tests = {str(include_passthru_tests).lower()};
}})""",
]

nix_eval = subprocess.run(cmd, stdout=subprocess.PIPE, text=True)
Expand Down
19 changes: 17 additions & 2 deletions nixpkgs_review/nix/evalAttrs.nix
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
{ attr-json }:
{ attr-json
, include-passthru-tests
}:

with builtins;
let
Expand Down Expand Up @@ -27,7 +29,7 @@ let
})
]
else
lib.flip map pkg.outputs or [ "out" ] (output:
(lib.flip map pkg.outputs or [ "out" ] (output:
let
# some packages are set to null if they aren't compatible with a platform or package set
maybePath = tryEval "${lib.getOutput output pkg}";
Expand All @@ -40,6 +42,19 @@ let
path = if !broken then maybePath.value else null;
drvPath = if !broken then pkg.drvPath else null;
}
))
++ lib.optionals include-passthru-tests (
lib.flip lib.mapAttrsToList pkg.passthru.tests or { } (test: drv:
let
maybePath = tryEval "${drv}";
broken = !exists || !maybePath.success;
in
lib.nameValuePair "${name}.passthru.tests.${test}" {
inherit exists broken;
path = if !broken then maybePath.value else null;
drvPath = if !broken then pkg.drvPath else null;
}
)
);
in

Expand Down
6 changes: 3 additions & 3 deletions nixpkgs_review/report.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ def __init__(
self.blacklisted.append(a)
elif not a.exists:
self.non_existent.append(a)
elif a.name.startswith("nixosTests."):
self.tests.append(a)
elif not a.was_build():
self.failed.append(a)
elif a.is_test():
self.tests.append(a)
else:
self.built.append(a)

Expand Down Expand Up @@ -189,5 +189,5 @@ def print_console(self, pr: int | None) -> None:
)
print_number(self.blacklisted, "blacklisted")
print_number(self.failed, "failed to build")
print_number(self.tests, "built", what="tests", log=print)
print_number(self.tests, "built", what="test", log=print)
print_number(self.built, "built", log=print)
39 changes: 37 additions & 2 deletions nixpkgs_review/review.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def __init__(
skip_packages_regex: list[Pattern[str]] = [],
checkout: CheckoutOption = CheckoutOption.MERGE,
sandbox: bool = False,
build_passthru_tests: bool = False,
) -> None:
self.builddir = builddir
self.build_args = build_args
Expand All @@ -119,6 +120,7 @@ def __init__(
self.build_graph = build_graph
self.nixpkgs_config = nixpkgs_config
self.extra_nixpkgs_config = extra_nixpkgs_config
self.build_passthru_tests = build_passthru_tests

def worktree_dir(self) -> str:
return str(self.builddir.worktree_dir)
Expand Down Expand Up @@ -205,6 +207,7 @@ def build(self, packages: set[str], args: str) -> list[Attr]:
self.system,
self.allow,
self.builddir.nix_path,
self.build_passthru_tests,
)
return nix_build(
packages,
Expand Down Expand Up @@ -409,12 +412,19 @@ def package_attrs(
allow: AllowedFeatures,
nix_path: str,
ignore_nonexisting: bool = True,
build_passthru_tests: bool = False,
) -> dict[str, Attr]:
attrs: dict[str, Attr] = {}

nonexisting = []

for attr in nix_eval(package_set, system, allow, nix_path):
for attr in nix_eval(
package_set,
system,
allow,
nix_path,
build_passthru_tests,
):
if not attr.exists:
nonexisting.append(attr.name)
elif not attr.broken:
Expand All @@ -434,14 +444,21 @@ def join_packages(
system: str,
allow: AllowedFeatures,
nix_path: str,
build_passthru_tests: bool,
) -> set[str]:
changed_attrs = package_attrs(changed_packages, system, allow, nix_path)
changed_attrs = package_attrs(
changed_packages,
system,
allow,
nix_path,
)
specified_attrs = package_attrs(
specified_packages,
system,
allow,
nix_path,
ignore_nonexisting=False,
build_passthru_tests=build_passthru_tests,
)

tests: dict[str, Attr] = {}
Expand Down Expand Up @@ -472,9 +489,24 @@ def filter_packages(
system: str,
allow: AllowedFeatures,
nix_path: str,
build_passthru_tests: bool,
) -> set[str]:
packages: set[str] = set()

# reduce number of times the passthru.tests are evaluated, by either doing
# it here or in join_packages
if build_passthru_tests and len(specified_packages) == 0:
changed_attrs = package_attrs(
changed_packages,
system,
allow,
nix_path,
build_passthru_tests=build_passthru_tests,
)
changed_packages = set(
changed_attrs[path].name for path in changed_attrs.keys()
Copy link
Owner

@Mic92 Mic92 Apr 25, 2024

Choose a reason for hiding this comment

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

Sorry for the late reply. I think because of the potential large memory consumption of nixos tests, we need to evaluate them isolation as they will otherwise make the build potentially go out of memory, i.e. if you try to have a large package set rebuild.
Therefore I would suggest to evaluate those packages ahead of time i.e. using nix-instantiate and add them to the final nix build. Of course a better design would be some sort of pipe-lining where already evaluated packages gets build as the rest of the evaluation is still in progress. But this probably requires bigger refactorings and could implemented at a later point similar to nix-fast-build. When using nix-instantiate also add a gcroot with --add-root to make sure that derivations are not garbage collected before hand.

)

if (
len(specified_packages) == 0
and len(package_regexes) == 0
Expand All @@ -490,6 +522,7 @@ def filter_packages(
system,
allow,
nix_path,
build_passthru_tests,
)

for attr in changed_packages:
Expand Down Expand Up @@ -560,6 +593,7 @@ def review_local_revision(
commit: str | None,
staged: bool = False,
print_result: bool = False,
build_passthru_tests: bool = False,
) -> Path:
with Builddir(builddir_path) as builddir:
review = Review(
Expand All @@ -578,6 +612,7 @@ def review_local_revision(
build_graph=args.build_graph,
nixpkgs_config=nixpkgs_config,
extra_nixpkgs_config=args.extra_nixpkgs_config,
build_passthru_tests=args.tests,
)
review.review_commit(builddir.path, args.branch, commit, staged, print_result)
return builddir.path