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

Avoid --impure in flake-parts based projects #1018

Merged
merged 2 commits into from
Apr 19, 2024
Merged

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Mar 20, 2024

Let .envrc generate the devenv.root

@Atry Atry mentioned this pull request Mar 20, 2024
@Atry Atry force-pushed the no-impure branch 3 times, most recently from 5efce28 to c69453e Compare March 20, 2024 20:15
@domenkozar
Copy link
Member

We should probably make it a JSON to pass other things like devenv.tmpdir?

@Atry
Copy link
Contributor Author

Atry commented Mar 21, 2024

We should probably make it a JSON to pass other things like devenv.tmpdir?

I think one value per input approach would be nice, otherwise we will have to deal with JSON escape things in .envrc

@Atry
Copy link
Contributor Author

Atry commented Mar 22, 2024

@domenkozar Would you like to approve the CI workflows?

templates/flake-parts/.envrc Outdated Show resolved Hide resolved
templates/flake-parts/flake.nix Outdated Show resolved Hide resolved
@khaled
Copy link

khaled commented Mar 22, 2024

Without these changes I get:

error:
       … while evaluating the attribute 'optionalValue.value'

         at /nix/store/lwyjz70qh12nq6cb7fixl85vryzxqm3c-source/lib/modules.nix:856:5:

          855|
          856|     optionalValue =
             |     ^
          857|       if isDefined then { value = mergedValue; }

       … while evaluating a branch condition

         at /nix/store/lwyjz70qh12nq6cb7fixl85vryzxqm3c-source/lib/modules.nix:857:7:

          856|     optionalValue =
          857|       if isDefined then { value = mergedValue; }
             |       ^
          858|       else {};

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: unable to download 'file://file:/dev/fd/63': URL using bad/illegal format or missing URL (3)

With the changes I get a further, but apparently there is still a problem:

$direnv allow
...
Hello, world!
ln: failed to create symbolic link '/Users/khaled/path/to/project'$'\n''/.devenv/run': No such file or directory

Oddly, the /path/to/project printed in the error message isn't the current path as output by pwd, but a different directory in which I previously tried the same experiment...

@Atry
Copy link
Contributor Author

Atry commented Mar 23, 2024

@khaled Fixed

@khaled
Copy link

khaled commented Mar 23, 2024

Thanks @Atry!

FYI, this probably belongs in a separate bug, but the existence of the packages.devenv-up package definition seems to actually break devenv up:

       error: attribute 'config' missing

       at /nix/store/86gyzipyn7zzp4nrnanz0sl5r6cps63v-source/flake.nix:48:30:

           47|         packages.default = pkgs.hello;
           48|         packages.devenv-up = self'.devShells.default.config.procfileScript;
             |                              ^
           49|         devenv.shells.default = let

Simply removing the package definition resolves it.

@Atry
Copy link
Contributor Author

Atry commented Mar 23, 2024

@domenkozar I don't understand the CI failure message. Could you help me understanding why?

@Atry
Copy link
Contributor Author

Atry commented Mar 27, 2024

@domenkozar I rebased this PR. Would you mind approving the CI workflows?

@Atry
Copy link
Contributor Author

Atry commented Mar 28, 2024

@domenkozar I think the CI failure is not due to this PR. The main branch is broken.

@lopter
Copy link
Contributor

lopter commented Apr 8, 2024

I am wondering if it would make sense to also update the flake integration wrapper to not use --impure anymore and maybe re-export other nix commands from there (e.g. the flake nix command).

@Atry
Copy link
Contributor Author

Atry commented Apr 8, 2024

@domenkozar could you trigger CI for this PR?

@domenkozar domenkozar merged commit 255fe24 into cachix:main Apr 19, 2024
230 of 251 checks passed
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.

None yet

4 participants