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

eval: remove is_test and embed mock-nixpkgs.nix #86

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

philiptaron
Copy link
Contributor

Motivation

Similar to #84, reducing the amount of needed machinery in Nix in favor of Rust code. In particular, remove NIX_PATH manipulation in Nix derivations!

Things Done

This commit does the following things:

  1. Purify the environment variables entirely when calling nix-instantiate. Opt in to the five environment variables needed in order to run nix-instantiate in the Nix sandbox.
  2. Remove the hard-coded NIX_PATH environment variables in default.nix and package.nix. These were implicitly sent through to nix-instantiate, along with -I arguments. Now, only -I is used.
  3. Replace the is_test argument with #[cfg(test)] applied to mutate_nix_instatiate_arguments_based_on_cfg.

While this is slightly more lines of code that the combination of Nix and Rust that it replaces, there is less Nix logic and more Rust logic.

Additionally, the Rust logic that is test-only is contained to cargo check only rather than being present in the production binary.

This commit does the following things:

1. Purify the environment variables entirely when calling `nix-instantiate`.
   Opt in to the five environment variables needed in order to run
   `nix-instantiate` in the Nix sandbox.
2. Remove the hard-coded `NIX_PATH` environment variables in `default.nix`
   and `package.nix`. These were implicitly sent through to `nix-instantiate`,
   along with `-I` arguments. Now, only `-I` is used.
3. Replace the `is_test` argument with `#[cfg(test)]` applied to
   `mutate_nix_instatiate_arguments_based_on_cfg`.

While this is slightly more lines of code that the combination of Nix and Rust
that it replaces, there is less Nix logic and more Rust logic.

Additionally, the Rust logic that is test-only is contained to `cargo check`
only rather than being present in the production binary.
@philiptaron philiptaron requested a review from a team as a code owner July 22, 2024 17:01
@philiptaron philiptaron changed the title eval.rs: remove is_test and embed mock-nixpkgs.nix eval: remove is_test and embed mock-nixpkgs.nix Jul 22, 2024
@philiptaron philiptaron requested a review from infinisil July 26, 2024 22:17
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Very neat, this is much cleaner!

"NIX_LOCALSTATE_DIR",
"NIX_LOG_DIR",
"NIX_STATE_DIR",
"NIX_STORE_DIR",
Copy link
Member

Choose a reason for hiding this comment

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

Note for the future: These probably aren't all needed, because we don't do any building, just evaluation. I suspect only NIX_STATE_DIR is necessary

@infinisil infinisil merged commit 749a7fc into NixOS:main Jul 26, 2024
3 checks passed
@philiptaron philiptaron deleted the integrate-test-packages branch August 29, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants