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

Search for imports in NICKEL_PATH #1716

Merged
merged 4 commits into from
Nov 22, 2023
Merged

Search for imports in NICKEL_PATH #1716

merged 4 commits into from
Nov 22, 2023

Conversation

jneem
Copy link
Member

@jneem jneem commented Nov 17, 2023

Here is a proposal for the most basic kind of "package management": a NICKEL_PATH environment whose value is a comma-separated list of paths look for imports in. The idea is that if you're using npm, for example, then you set this up to point to node_modules and then nickel can find all your imported packages.

It works reasonably well with nix also, as long as there are no recursive dependencies. For example, I'm using the following flake to get nickel validation for helix configurations:

{
  description = "Helix and nickel";

  inputs = {
    helix-nickel.url = "github:jneem/helix-nickel";
    nickel.url = "github:tweag/nickel/nickel-path";
  };

  outputs = { self, nixpkgs, helix-nickel, nickel }:
  let pkgs = import nixpkgs {
      system = "x86_64-linux";
    };
  in
  {
    devShell.x86_64-linux = pkgs.mkShell {
      NICKEL_PATH = helix-nickel.packages.x86_64-linux.default;

      nativeBuildInputs = [
        nickel.packages.x86_64-linux.default
      ];
    };
  };
}

I didn't have a good idea for how to test this, given that it requires setting up an environment variable...

@github-actions github-actions bot temporarily deployed to pull request November 17, 2023 05:14 Inactive
Comment on lines 327 to 329
let import_paths = std::env::var("NICKEL_PATH")
.map(|paths| paths.split(':').map(PathBuf::from).collect())
.unwrap_or_default();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should rather be an argument to Cache::new and then the actual logic from where to get the path should be up to the particular use case.

Copy link
Member Author

@jneem jneem Nov 17, 2023

Choose a reason for hiding this comment

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

Putting this in Cache::new required threading them through a lot of things, so I tried out making the import paths mutable after construction. This changes the behavior a little, but I think it might be desirable actually: when running nix export file.nix, the path "file.nix" is now only searched for in the current directory, and NICKEL_PATH is only used for finding things that "file.nix" imports.

Edit: also, this will make tests easier to write. I'll add those when I have a little more time. (done)

@github-actions github-actions bot temporarily deployed to pull request November 17, 2023 15:49 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 17, 2023 20:34 Inactive
@jneem jneem requested a review from vkleen November 17, 2023 20:35
@github-actions github-actions bot temporarily deployed to pull request November 17, 2023 20:38 Inactive
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

Naive question: is there any rationale behind using an environment variable versus a CLI argument --nickel-path? Or versus using both? (whatever is the answer, it can be treated in a separate PR anyway)

@jneem
Copy link
Member Author

jneem commented Nov 22, 2023

I think probably it makes sense to have both. Maybe @thufschmitt will have an opinion, since I think he's the one that asked for the env var originally.

@jneem jneem added this pull request to the merge queue Nov 22, 2023
Merged via the queue into master with commit f7ffe31 Nov 22, 2023
5 checks passed
@jneem jneem deleted the nickel-path branch November 22, 2023 17:11
@thufschmitt
Copy link
Contributor

Oh my! This is awesome, it means that we can get rid of the nickel lockfile in Organist.

I'm personally mostly interested in the environment variable, but a CLI flag would probably make sense for a number of use-cases.

@uhlajs
Copy link

uhlajs commented Dec 1, 2023

This is great! We already have a PoC for nickel-lang/rules_nickel/, which adds imports keyword to nickel_export (similar to py_binary.imports). By doing this, we are able to eliminate the ugly relative path references in our imports.

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.

5 participants