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.

Sign up for GitHub

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

bats: improve package; move installCheck to passthru #170019

Merged
merged 2 commits into from
May 16, 2022
Merged

bats: improve package; move installCheck to passthru #170019

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
749b97b
bats: improve package w/ new resholve features
abathur Apr 23, 2022
a60c1db
bats: move installCheck into passthru.tests
abathur Apr 23, 2022
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
99 changes: 86 additions & 13 deletions pkgs/development/interpreters/bats/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@
, coreutils
, gnugrep
, ncurses
, findutils
, hostname
, parallel
, flock
, ps
, bats
, lsof
, doInstallCheck ? true
}:
Expand All @@ -31,32 +37,99 @@ resholve.mkDerivation rec {

solutions = {
bats = {
scripts = [ "bin/bats" ];
scripts = [
"bin/bats"
"libexec/bats-core/*"
"lib/bats-core/*"
];
interpreter = "${bash}/bin/bash";
inputs = [ bash coreutils gnugrep ];
inputs = [
bash
coreutils
gnugrep
ncurses
findutils
hostname
parallel
flock
"lib/bats-core"
"libexec/bats-core"
];
fake = {
external = [ "greadlink" ];
external = [
"greadlink"
"shlock"
];
};
fix = {
"$BATS_ROOT" = [ "${placeholder "out"}" ];
"$BATS_LIBEXEC" = [ "${placeholder "out"}/libexec/bats-core" ];
};
keep = {
"${placeholder "out"}/libexec/bats-core/bats" = true;
source = [
"${placeholder "out"}/lib/bats-core/validator.bash"
"${placeholder "out"}/lib/bats-core/preprocessing.bash"
"$BATS_TEST_SOURCE"
"${placeholder "out"}/lib/bats-core/tracing.bash"
"${placeholder "out"}/lib/bats-core/test_functions.bash"
"$library_load_path"
"${placeholder "out"}/lib/bats-core/common.bash"
"${placeholder "out"}/lib/bats-core/semaphore.bash"
"${placeholder "out"}/lib/bats-core/formatter.bash"
];
"$report_formatter" = true;
"$formatter" = true;
"$pre_command" = true;
"$BATS_TEST_NAME" = true;
"${placeholder "out"}/libexec/bats-core/bats-exec-test" = true;
};
execer = [
/*
both blatant lies for expedience; these can certainly exec args
they may be safe here, because they may always run things that
are ultimately in libexec?
TODO: handle parallel and flock in binlore/resholve
*/
"cannot:${parallel}/bin/parallel"
"cannot:${flock}/bin/flock"

"cannot:libexec/bats-core/bats-preprocess"

# these do exec, but other internal files
"cannot:libexec/bats-core/bats-exec-file"
"cannot:libexec/bats-core/bats-exec-suite"
];
};
};

inherit doInstallCheck;
installCheckInputs = [ ncurses ] ++ lib.optionals stdenv.isDarwin [ lsof ];
installCheckPhase = ''
# TODO: cut if https://github.com/bats-core/bats-core/issues/418 allows
sed -i '/test works even if PATH is reset/a skip' test/bats.bats
passthru.tests.upstream = bats.unresholved.overrideAttrs (old: {
name = "${bats.name}-tests";
installCheckInputs = [
ncurses
parallel # skips some tests if it can't detect
flock # skips some tests if it can't detect
ps
] ++ lib.optionals stdenv.isDarwin [ lsof ];
inherit doInstallCheck;
Copy link
Member

Choose a reason for hiding this comment

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

All of this should not be in passtruh and only doCheck rule be overwritten.

Copy link
Member Author

@abathur abathur May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is one of the main points of the PR, so I'm inclined to push back on this without sources/reasoning.

While this approach is rare, I don't think it's unprecedented within nixpkgs. It's also roughly aligned with the WIP recommendations in NixOS/rfcs#119 (specifically https://github.com/NixOS/rfcs/pull/119/files#diff-d207098de17afa3aaea2ef0e420b2b1c674b257267828c11e40928351df3e368R46-R54).

Copy link
Member

Choose a reason for hiding this comment

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

I mean like https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/sortedcontainers/default.nix#L27-L29

Copy link
Member Author

@abathur abathur May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah. That makes sense, now--though it doesn't work in this case.

I may have mis-designed resholve's Nix API, but it produces an inner derivation straightforwardly, and then uses it as the source for an outer derivation that is passed through resholve.

I'm not sure if that is at all persuasive, but resholve's Nix API and this test can be incrementally improved in subsequent PRs.

Edit:
It may help if I am more concrete...

The test here overrides the inner (unresholved) output as a kludge to get access to the normal build's tests directory, but then it's using the outer (resholved) output to actually run the those tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SuperSandro2000 ping? thoughts? I can also bug someone else if you're short on time :)

installCheckPhase = ''
# TODO: cut if https://github.com/bats-core/bats-core/issues/418 allows
sed -i '/test works even if PATH is reset/a skip "disabled for nix build"' test/bats.bats
# TODO: cut when https://github.com/bats-core/bats-core/pull/554 allows
substituteInPlace test/parallel.bats --replace '&& type -p shlock' '|| type -p shlock'

# test generates file with absolute shebang dynamically
substituteInPlace test/install.bats --replace \
"/usr/bin/env bash" "${bash}/bin/bash"
bin/bats test
'';
# skip tests that assume bats `install.sh` will be in BATS_ROOT
rm test/root.bats

# test generates file with absolute shebang dynamically
substituteInPlace test/install.bats --replace \
"/usr/bin/env bash" "${bash}/bin/bash"

${bats}/bin/bats test
rm -rf $out
touch $out
'';
});

meta = with lib; {
homepage = "https://github.com/bats-core/bats-core";
Expand Down