pytestCheckHook: invoke the pytest command directly#417836
pytestCheckHook: invoke the pytest command directly#417836ShamrockLee wants to merge 3 commits intoNixOS:stagingfrom
Conversation
Invoke the pytest executable as `pytest` instead of `python -m pytest` to prevent additional Python module search paths (such as CWD) from prefixing to `sys.paths`.[1] [1]: https://docs.python.org/3/library/sys.html#sys.path
|
Tested on the This works: nix build --no-link --print-out-paths -L --impure --expr '
let
urlNixpkgs = "github:NixOS/nixpkgs";
nixpkgsBase = builtins.getFlake "${urlNixpkgs}/nixos-unstable";
lib = nixpkgsBase.lib;
pkgs = nixpkgsBase.legacyPackages.${builtins.currentSystem};
nixpkgsChanged = builtins.getFlake "${urlNixpkgs}/refs/pull/417836/head";
python3PackagesHooks = (
pkgs.python3.override {
packageOverrides = import "${nixpkgsChanged}/pkgs/development/interpreters/python/hooks/default.nix";
}
).pkgs;
in (
pkgs.python3Packages.dtw-python.override {
inherit (python3PackagesHooks) pytestCheckHook;
}
).overridePythonAttrs (_: {
preCheck = "";
})
'This fails: nix build --no-link --print-out-paths -L --impure --expr '
let
urlNixpkgs = "github:NixOS/nixpkgs";
nixpkgsBase = builtins.getFlake "${urlNixpkgs}/nixos-unstable";
lib = nixpkgsBase.lib;
pkgs = nixpkgsBase.legacyPackages.${builtins.currentSystem};
in
pkgs.python3Packages.dtw-python.overridePythonAttrs (_: {
preCheck = "";
})
' |
github-actions
bot
added
10.rebuild-darwin: 501+
|
Is the plan to do a cleanup of the workaround afterwards? |
Sure. |
|
The following is a list of potentially affected expressions found via |
Could you please also perform a few tests of packages that don't use |
@ofborg build python3Packages.pip |
|
This change causes The directory layout of the purl project is: The |
You have learned something from this? Since this PR targets staging, I can't see how ofborg would be able to reach building those packages and not timeout, if there are many packages to build on the way to these because we target
I'm afraid that this is the kind of breakages I was afraid of. Maybe @FRidh will be able to explain this phenomena, and justify the choice of |
|
Just wanted to add some extra info: Also, here's my take on the script to search for potential files (needs ripgrep installed): rg "pytestCheckHook" -l | xargs rg "preCheck" -l | xargs rg " cd | pushd | rm | mv " -lSwap out the last |
This inspires me to automatically add "source modules" right under CWD matching the installed modules to
|
ShamrockLee
force-pushed
the
pytest-execute-directly
branch
from
a5a9d60 to
1329f42
Compare
doronbehar
left a comment
doronbehar
left a comment
There was a problem hiding this comment.
Main changes look pretty good, but this needs thorough testing.
|
|
||
| : To append additional command-line arguments to `pytest`. | ||
|
|
||
| `dontDisableSourceMoudleTestPaths` |
There was a problem hiding this comment.
Maybe:
| `dontDisableSourceMoudleTestPaths` | |
| enableSourceMoudleTestPaths |
doc/release-notes/rl-2511.section.md
Outdated
| - `pytestCheckHook` now invokes the `pytest` command directly instead of using `python3 -m pytest`, preventing the current working directory of the `pytestCheckPhase` (usually the build directory) from becoming the first directory to search for modules. | ||
| The installed site packages directory now becomes the first directory to search for modules, and Python packages that need to test against the built and installed package no longer need to `cd` to another directory before testing. | ||
|
|
||
| In addition, `pytestCheckHook` now adds the source module into `disabledTestPaths` if they are right under the current working directory, preventing import mismatch when using the installed modules for testing. |
There was a problem hiding this comment.
I'd couple these 2 changes to a single commit, and also join the paragraphs.
| The installed site packages directory now becomes the first directory to search for modules, and Python packages that need to test against the built and installed package no longer need to `cd` to another directory before testing. | ||
|
|
||
| In addition, `pytestCheckHook` now adds the source module into `disabledTestPaths` if they are right under the current working directory, preventing import mismatch when using the installed modules for testing. | ||
| Python packages no longer need to delete the source modules before running `pytest`. |
There was a problem hiding this comment.
cd $out is a more common workaround, so I'd mention it too, and before mentioning the deletion.
|
Bad news: Further tests shows that By swapping the source module with a stub (a directory with the same name containing nothing but an empty By inserting the line |
nixpkgs-ci
bot
added
the
2.status: merge conflict
which tests are you running. Maybe there exists tests right under /build/source, hence pytest will append the directory |
This comment slipped under my radar. I'll prepare a reproducer later. |
5 participants
This PR make
pytestCheckHookinvoke the pytest executable aspytestinstead ofpython -m pytestto prevent additional Python module search paths (such as CWD) from prefixing tosys.paths.1 In addition,pytestCheckHooknow adds the source module intodisabledTestPathsif they are right under the CWD, preventing import mismatch when using the installed modules for testing.The installed site packages directory now becomes the first directory to search for modules, and Python packages that need to test against the built and installed package no longer need to
cdto another directory or delete the source modules before testing.Closes #255262
Cc:
@doronbehar who proposed the fix
@FRidh who introduced the original pattern
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.