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

Aadd --impure flag when calling print-dev-env #2279

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Conversation

imfede
Copy link
Contributor

@imfede imfede commented Sep 17, 2024

Summary

Add --impure to the print-dev-env command, allowing flakes to access environment variables also in this step. Without this change, a flake that needs an env var to work will correctly build but then a shell could not be created.

This also solves #2196, signifying that other people encountered a similar problem.

How was it tested?

Both by

devbox run test

and manually, building the tool and locally checking that a flake would pick up variables (with builtins.getEnv) when activating a shell.

@mikeland73
Copy link
Contributor

@imfede, thanks for the contribution!

IIRC we didn't make this impure in order to use the nix evaluation cache. That said, devbox uses its own caching as well, so we might not need that.

Could you add a feature flag to this and default it to disabled? That way you could start using it right away and we can test if it has any perf side-effects.

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.

Please put this behind a disabled feature flag.

See featureflag package.

To enable for yourself you can use

DEVBOX_FEATURE_[NAME]=1

After we test a bit, if we don't see a performance issue we'll remove the flag and make it permanent.

Thanks again for contribution!

@imfede
Copy link
Contributor Author

imfede commented Sep 19, 2024

Thanks for the review!

My golang skills are practically non-existant, so I hope the end result is good enough. Let me know how can I improve on it if needed :)

if featureflag.ImpurePrintDevEnv.Enabled() {
optionalImpureFlag = "--impure"
}
cmd := command("print-dev-env", "--json", optionalImpureFlag,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I do the following steps, then I get an error here:

# build binary in dist/devbox
 % devbox run build
 
 # clear local state
 % rm -rf .devbox
 
 # install the devbox-repo's own packages
 % dist/devbox install
Info: Ensuring packages are installed.
✓ Computed the Devbox environment.
Error: nix: command error: nix --extra-experimental-features ca-derivations --option experimental-features 'nix-command flakes fetch-closure' print-dev-env --json  path:/Users/savil/code/jetpack/devbox/.devbox/gen/flake: unexpected argument 'path:/Users/savil/code/jetpack/devbox/.devbox/gen/flake': exit code 1

Error: There was an internal error. Run with DEVBOX_DEBUG=1 for a detailed error message, and consider reporting it at https://github.com/jetify-com/devbox/issues

I think the issue is with the empty argument.

I think you could do:

		cmd := command("print-dev-env", "--json")
		if featureflag.ImpurePrintDevEnv.Enabled() {
			cmd.Args = append(cmd.Args, "--impure")
		}
		cmd.Args = append(cmd.Args, "path:"+flakeDirResolved)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, I thought I had tested it without the feature flag active.

Thanks for spotting this, I have pushed a fix following your suggestion.

@imfede imfede requested a review from savil September 20, 2024 16:44
Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

LGTM thanks for sending this!

@savil savil dismissed mikeland73’s stale review September 20, 2024 17:50

Mike, said lgtm in our chat program

@savil savil merged commit c314509 into jetify-com:main Sep 20, 2024
23 of 24 checks passed
@imfede
Copy link
Contributor Author

imfede commented Sep 23, 2024

Thanks for the reviews! Out of information, do you have a plan on when this could be released?

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.

3 participants