Make the review-shell purer
#373
Conversation
Artturin
force-pushed
the
makemorepure
branch
from
d79d872
to
760e7d1
Compare
There was a problem hiding this comment.
@Mic92 suggested that the packages = if builtins.length paths > 50 then [ env ] else paths; should be changed to use the buildEnv always
Issues with that
There's no python3.11 in PATH when using buildEnv because it doesn't take into account the propagatedBuildInputs when reviewing PR 261696 a python module bump
There's no $PYTHONPATH or other variables set because no hooks
However, are we sure that those aren't good things? Binaries will fail to launch if they're missing deps(good) and reviewing by importing modules is often not useful because there's already a pythonImportsCheck in python packages.
The hooks just bring side effects which shouldn't be relied on outside nix builds, nixpkgs-review should be testing without side effects
There was a problem hiding this comment.
"/${python3.sitePackages}"
could be in buildEnv,
and in the review-shell
PYTHONPATH = "${env}/${python3.sitePackages};
and python3 appended to packages
There was a problem hiding this comment.
What would be than our suggested workflow for to users of the tool to test libraries? Just now they can use the normal nix-shell workflow.
The purpose of this `env` is to add the binaries to the `PATH`. The benefit of making it purer is that unwrapped programs missing dependencies may fail. As a added benefit this dramatically reduces the collision warnings from `buildEnv` `warning: collision between '/nix/store/82icynaidvzyic2g349cqi4p9a5d4aa2-vimplugin-ai.vim-2023-10-03/doc/tags' and '/nix/store/qc4bdwi6zhk4r1vzbcxrnwrrjnm33a6g-vimplugin-CheckAttach-2019-05-08/doc/tags'`
|
@mergify queue |
🛑 The pull request has been removed from the queue
|
|
@Mergifyio refresh |
✅ Pull request refreshed |
|
@mergify queue |
🛑 The pull request has been removed from the queue
|
The only purpose of this
env(I assume) is to add the binaries to thePATH.The benefit of making it purer is that unwrapped programs missing dependencies may fail.
As a added benefit this dramatically reduces the collision warnings from
buildEnvwarning: collision between '/nix/store/82icynaidvzyic2g349cqi4p9a5d4aa2-vimplugin-ai.vim-2023-10-03/doc/tags' and '/nix/store/qc4bdwi6zhk4r1vzbcxrnwrrjnm33a6g-vimplugin-CheckAttach-2019-05-08/doc/tags'Closes #370