Skip to content

[remove nixpkgs] Remove CAPath, and only enable for nix >= 2.17 #1332

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

Merged
merged 5 commits into from
Aug 8, 2023

Conversation

savil
Copy link
Collaborator

@savil savil commented Jul 29, 2023

Summary

We want to restrict the feature to nix 2.17+, because it enables pure mode nix
when using just fromPath in builtins.fetchClosure.

This greatly simplifies the devbox.lock issues with caPath, and will improve the UX.

This Remove Nixpkgs feature is most beneficial to new users who have not downloaded the nixpkgs they need. These users are most likely to have the latest nix installed for them, so I think restricting to nix >= 2.17 is an acceptable tradeoff.

How was it tested?

did a sanity test on x86-darwin.
need to test better on a "supported system" where nix >= 2.17:

  • updated Devbox Cloud to run nix 2.17
  • in examples/development/python/pip, did devbox shell. Since we have system-info for python and not pip, the generated flake.nix looked like this:
{
   description = "A devbox shell";

   inputs = {
     nixpkgs.url = "http://lax.devbox-nixed.internal/nixos/nixpkgs/archive/8670e496ffd093b60e74e7fa53526aa5920d09eb.tar.gz";
     nixpkgs-8670e4.url = "http://lax.devbox-nixed.internal/nixos/nixpkgs/archive/8670e496ffd093b60e74e7fa53526aa5920d09eb.tar.gz";
   };

   outputs = {
     self,
     nixpkgs,
     nixpkgs-8670e4,
   }:
      let
        pkgs = nixpkgs.legacyPackages.x86_64-linux;
        nixpkgs-8670e4-pkgs = (import nixpkgs-8670e4 {
          system = "x86_64-linux";
          config.allowUnfree = true;
          config.permittedInsecurePackages = [
          ];
        });
      in
      {
        devShells.x86_64-linux.default = pkgs.mkShell {
          buildInputs = [
            (builtins.fetchClosure{
              fromStore = "https://cache.nixos.org";
              fromPath = "/nix/store/95cxzy2hpizr23343b8bskl4yacf4b3l-python3-3.10.11";
              inputAddressed = true;
            })
            nixpkgs-8670e4-pkgs.python310Packages.pip
          ];
        };
      };
 }

Copy link
Collaborator Author

savil commented Jul 29, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil requested a review from ipince July 29, 2023 00:09
@savil savil requested review from gcurtis and mikeland73 August 2, 2023 19:02
@savil savil marked this pull request as draft August 2, 2023 23:46
@savil
Copy link
Collaborator Author

savil commented Aug 2, 2023

Moving to draft. From testing this on devbox-cloud with nix 2.17, I see some errors that need fixing.

@savil savil marked this pull request as ready for review August 3, 2023 17:58
Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

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

nice!

Comment on lines 147 to 159
func Version() (string, error) {
cmd := command("--version")
outBytes, err := cmd.Output()
if err != nil {
return "", errors.WithStack(err)
}
out := string(outBytes)
const prefix = "nix (Nix) "
if !strings.HasPrefix(out, prefix) {
return "", errors.Errorf(`Expected "%s" prefix, but output from nix --version was: %s`, prefix, out)
}
return strings.TrimSpace(strings.TrimPrefix(out, prefix)), nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we cache this output? In a package level variable is probably fine.

@savil savil merged commit f53929d into main Aug 8, 2023
@savil savil deleted the savil/rm-nixpkgs-new-nix branch August 8, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants