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

Make the review-shell purer #373

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make the review-shell purer #373

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5639831
Make the `review-shell` purer
Artturin Oct 20, 2023
08df34c
always use the buildEnv
Artturin Oct 22, 2023
9c77236
fix missing buildEnv
Mic92 Dec 9, 2023
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
13 changes: 8 additions & 5 deletions nixpkgs_review/nix.py
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator Author

@Artturin Artturin Oct 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"/${python3.sitePackages}"
could be in buildEnv,
and in the review-shell
PYTHONPATH = "${env}/${python3.sitePackages};
and python3 appended to packages

Copy link
Owner

@Mic92 Mic92 Oct 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -300,26 +300,29 @@ def write_shell_expression(
with open(filename, "w+", encoding="utf-8") as f:
f.write(
f"""{{ pkgs ? import ./nixpkgs {{ system = \"{system}\"; config = import {nixpkgs_config}; }} }}:
with pkgs;

let
paths = [
paths = with pkgs; [
"""
)
f.write("\n".join(f" {escape_attr(a)}" for a in attrs))
f.write(
"""
];
env = buildEnv {
hostPkgs = (import ./nixpkgs { });
env = hostPkgs.buildEnv {
name = "env";
inherit paths;
pathsToLink = [ "/bin" ];
ignoreCollisions = true;
};
in (import ./nixpkgs { }).mkShell {
in hostPkgs.mkShell {
name = "review-shell";
preferLocalBuild = true;
allowSubstitutes = false;
dontWrapQtApps = true;
packages = if builtins.length paths > 50 then [ env ] else paths;
# with a separate buildEnv the impurities such as hooks and propagatedBuildInputs won't be added to the shell
packages = [ env ];
}
"""
)
Loading