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

feat(dotenv): allow passing full paths to dotenv files #1080

Closed
wants to merge 0 commits into from

Conversation

alirezamirsepassi
Copy link

This pull request enables the dotenv integration to accept full paths to the dotenv files. Additionally, I've changed the option filename to files and moved the normalize function to the option itself using lib.toList. I've also added a warning regarding the updated option name to prevent users from breaking their environments.

P.S. I chose to use config.devenv.root instead of self because self evaluates to the nix store source tree when used in a flake configuration. Since dotenv files are typically not committed to a git repository, they won't be present after the copy.

files = lib.mkOption {
type = lib.types.either lib.types.path (lib.types.listOf lib.types.path);
apply = lib.toList;
default = config.devenv.root + "/.env";
Copy link
Contributor

@shyim shyim Mar 28, 2024

Choose a reason for hiding this comment

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

I don't think allowing any path is good. This would make it more hard to remove impure nix mode, as we can access files outside of our project 🤔

Aren't relative paths fine inside your project 🤔

Copy link
Author

@alirezamirsepassi alirezamirsepassi Mar 28, 2024

Choose a reason for hiding this comment

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

Thanks for your response, I appreciate it. I agree with your point of view, but after doing some research, I found out that flakes copy the source to Nix store before evaluating it, and there's no way around this. As a result, using a git ignored dotenv file is impossible. 😭

Nevertheless, I do think that allowing users to choose the dotenv file path, even if it's outside of the project, is a great idea as it provides more flexibility. For example, I often have variables that I need to share across multiple projects, and a single dotenv file can be used for this purpose.

Aren't relative paths fine inside your project

No, as I mentioned the dotenv file is in my .gitignore file and it's not being copied to the nix store unfortunately

wdyt? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

The project should not be copied into nix store with devenv provided nix 🤔 . But yes the gitignore has an effect 😓 .

Maybe we find a general solution to .gitignore, because of other issues like #1078

Copy link
Author

Choose a reason for hiding this comment

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

How does the devenv CLI create the shell btw? 🤔 I'm not very familiar with rust honestly but what I understood from here, is it runs nix develop with some options and arguments, right? 🤔 does it copy ignored files as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

You stay in your local normal copy where you did run devenv shell. I am right now looking if to improve this 😅

Copy link
Author

Choose a reason for hiding this comment

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

@domenkozar hi there!

I saw that you changed config.devenv.root to self in the rewrite in rust project, this, unfortunately, breaks my environments where I use flakes as I mentioned in the original description of this PR. Do you happen to know the command that devenv runs to create the shell? Maybe including that command in the .envrc file along with the flakes template would make sense as in: currently the dotenv integration doesn't work in combination with flakes. wdyt? 🤔

Copy link

@mschnee mschnee Mar 28, 2024

Choose a reason for hiding this comment

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

Also confirmed that the rust rewrite has broken dotenv support in devenv when using flakes. For now, I'm using the python-rewrite branch.

flake-test via ❄ impure (devenv-shell-env) …
➜ direnv reload
direnv: loading ~/Projects/flake-test/.envrc
direnv: loading https://raw.githubusercontent.com/nix-community/nix-direnv/2.2.1/direnvrc (sha256-zelF0vLbEl5uaqrfIzbgNzJWGmLzCmYAkInj/LNxvKs=)
direnv: using flake . --impure
direnv: nix-direnv: renewed cache
💡 The dotenv file '.env' was not found.

Using the default flake created by nix flake init --template github:cachix/devenv as documented here.

{
  inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/nixos-23.11";
    systems.url = "github:nix-systems/default";
    devenv.url = "github:cachix/devenv";
    devenv.inputs.nixpkgs.follows = "nixpkgs";
  };

  nixConfig = {
    extra-trusted-public-keys = "devenv.cachix.org-1:w1cLUi8dv3hnoSPGAuibQv+f9TZLr6cv/Hm9XgU50cw=";
    extra-trusted-substituters = "https://devenv.cachix.org";
  };

  outputs = { self, nixpkgs, devenv, systems, ... } @ inputs:
    let
      forEachSystem = nixpkgs.lib.genAttrs (import systems);
    in
    {
      packages = forEachSystem (system: {
        devenv-up = self.devShells.${system}.default.config.procfileScript;
      });

      devShells = forEachSystem
        (system:
          let
            pkgs = nixpkgs.legacyPackages.${system};
          in
          {
            default = devenv.lib.mkShell {
              inherit inputs pkgs;
              modules = [
                {
                  dotenv.enable = true;
                  # https://devenv.sh/reference/options/
                  packages = [ pkgs.hello ];

                  enterShell = ''
                    hello
                  '';

                  processes.run.exec = "hello";
                }
              ];
            };
          });
    };
}

Temporary Solution for Flakes
Use the python-rewrite branch (shrug)

inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/nixos-23.11";
    systems.url = "github:nix-systems/default";
    devenv.url = "github:cachix/devenv/python-rewrite";
    devenv.inputs.nixpkgs.follows = "nixpkgs";
  };

Copy link
Member

Choose a reason for hiding this comment

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

@shyim can you try using this PR to see if it addresses #257

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use dotenv module and I don't see it why it should improve the performance 🤔

Copy link
Member

Choose a reason for hiding this comment

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

It would still evaluate self that should be impacting the performance

@domenkozar
Copy link
Member

This breaks on devenv with:

       error:
       error: A definition for option `dotenv.files."[definition 1-entry 1]"' is not of type `path'. Definition values:
       - In `/nix/store/virtual0000000000000000000000007-source/src/modules/integrations/dotenv.nix': ".env"

because the semantics of the paths are different, I'll think about it how to properly integrate it.

@alirezamirsepassi
Copy link
Author

Hey @domenkozar

Thanks for your reply.

IMO, the path type isn't very important for the module and it can be switched back to a simple string as the file path will be verified to exist later in the module using pathExists anyway.

On second thought, we might not even want to have the path type as it will be copied to the Nix store if I'm not mistaken and it might contain some secrets that can't be managed directly/easily on the machine which later can be extracted by others. 🤔

let me know what you think. 🙏🏻

@alirezamirsepassi
Copy link
Author

@domenkozar I switched the type back to string and wrapped the default value in a double quotation to avoid nix copying it to the store.

Could you try again on devenv, please?

@alirezamirsepassi
Copy link
Author

@domenkozar any updates on this?

@domenkozar
Copy link
Member

Thanks for the ping, CI is running

@domenkozar
Copy link
Member

nix build
PATH=$PWD/result/bin:$PATH ./result/bin/devenv-run-tests --only dotenv tests

I get

error: string '.env' doesn't represent an absolute path

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.

4 participants