Add --envrc-dir flag to allow specifying location of direnv config #2629

pstephan-geico-external · 2025-06-11T19:45:30Z

Summary

Fixes #2459 - devbox generate direnv does not respect --config

It seems that the previous mode of operation for devbox generate direnv --config <some-dir> used --config to determine the location of the resulting .envrc file, rather than as the location of the devbox.json file. But in some cases it is desired to be able to specify not only the location of not only the direnv .envrc file, but also the devbox config file. Or at least to be able to specify just the location of the devbox config with the .envrc being in the current directory.

To accomplish this, without breaking the current mode of operation, a new parameter is added, --envrc-dir, which specifies the location of the resulting .envrc file. For example, the command devbox generate direnv --envrc-dir ABC --config ABC/XYZ would create ABC/.envrc and within that would be the command eval "$(devbox generate direnv --print-envrc --config XYZ)"

Without the new --envrc-dir param, operation is the same as it was previously.

The output from --print-envrc now invokes devbox shellenv... prior to the watch_file command. This allows us to use $DEVBOX_PROJECT_ROOT to specify the path to the config and lock files. This ensures the correct files are being watched.

How was it tested?

Several new tests have been created to cover cases with and without the new --envrc-dir param.

Community Contribution License

All community contributions in this pull request are licensed to the project
maintainers under the terms of the
Apache 2 License.

By creating this pull request, I represent that I have the right to license the
contributions to the project maintainers under the Apache 2 License as stated in
the
Community Contribution License.

darch-geico-external

I think I only spotted one actual bug.

internal/boxcli/generate.go

internal/devbox/devbox.go

internal/devbox/generate/devcontainer_util.go

testscripts/generate/direnv-printenvrc.test.txt

internal/devbox/devbox.go

darch-geico-external

Obviously still needs review from an actual maintainer, but I don't see any other issues from here.

jguluarte

LGTM

internal/boxcli/generate.go

Copilot

Pull Request Overview

Adds a new --envrc-dir flag to the devbox generate direnv command so users can specify where the .envrc file is written separately from the devbox config.
Key changes:

Introduce EnvrcOpts to carry both EnvrcDir and ConfigDir through internal APIs
Update templates (envrc.tmpl, envrcContent.tmpl) to respect both flags
Extend CLI (boxcli/generate.go), backend logic (determineDirenvDirs), and tests to cover --envrc-dir behavior

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
testscripts/generate/*.test.txt	Added and updated shells scripts to validate new `--envrc-dir` flag
internal/devbox/generate/tmpl/envrcContent.tmpl	Updated template to prefix files and include `--config` when set
internal/devbox/generate/tmpl/envrc.tmpl	Adjusted embedded `eval` invocation to emit `--config` if needed
internal/devbox/generate/devcontainer_util.go	Refactored `CreateEnvrc` to take `EnvrcOpts` and write to `EnvrcDir`
internal/devbox/devopt/devboxopts.go	Introduced `EnvrcOpts` struct embedding `EnvFlags`
internal/devbox/devbox.go	Updated `GenerateEnvrcFile`, `PrintEnvrcContent`, and related flows
internal/boxcli/generate.go	Registered `--envrc-dir` flag, wired into `runGenerateDirenvCmd`

Comments suppressed due to low confidence (2)

internal/devbox/generate/tmpl/envrcContent.tmpl:1

[nitpick] The template variable .Dir is ambiguous; consider renaming it to .ConfigDir so its purpose is clearer in both templates.

{{define "DirPrefix"}}{{ if .Dir }}{{ .Dir }}/{{ end }}{{end}}

internal/devbox/devbox.go:558

[nitpick] When opts.EnvrcDir is empty, this prints "", which may confuse users. Consider special-casing the message to say "current directory" instead of empty quotes.

	ux.Fsuccessf(d.stderr, "generated .envrc file in %q.\n", opts.EnvrcDir)

internal/boxcli/generate.go

mikeland73

This is a cool feature!

Two main changes I think you should do:

--envrc-dir should not control/override --config. It should be independent.
Don't pre-compute relative path between the dirs. Keep existing path semantics and only compute the relative path when you need to actually pting out the .envrc

mikeland73 · 2025-07-02T00:36:10Z

internal/boxcli/generate.go

+		"path to directory where the .envrc file should be generated. "+
+			"If not specified, the .envrc file will be generated in "+
+			"the current working directory.")
+


Should be:

"If not specified, the .envrc file will be generated in "+ "the same directory as the devbox.json")

Since --config will determine where the .envrc file is created, which may not match the working directory.

Maybe also clarify that if this flag is used, then we use relative path for --config

Good Catch and good suggestion.

mikeland73 · 2025-07-02T00:42:06Z

internal/boxcli/generate.go

+	// If no configDir is specified, it will be assumed to be in the same directory as the .envrc file
+	// which means we can just return an empty configDir.
+	if configDir == "" {
+		return "", envrcDir, nil
+	}


I'm not sure I agree with the semantics of this. --envrc-dir should dictate where the .envrc is created, not which config is used. The default for all devbox commands is the use the devbox.json in the current directory and if it doesn't exist, recursively check parent directories. I think we should preserve that.

In practice that means that using --envrc-dir and not using configDir will use the "current' devbox project, but create the .envrc where specified.

Agreed. Change made.

mikeland73 · 2025-07-02T00:49:46Z

internal/boxcli/generate.go

+	relativeConfigDir, err := filepath.Rel(envrcDir, configDir)
+	if err != nil {
+		return "", "", errors.Wrapf(err, "failed to determine relative path from %s to %s", envrcDir, configDir)
+	}
+
+	// If the relative path is ".", it means configDir is the same as envrcDir. Leaving it as "."
+	// will result in the .envrc containing "--config .", which is fine, but unnecessary and also
+	// a change from the previous behavior. So we will return an empty string for relativeConfigDir
+	// which will result in the .envrc file not containing the "--config" flag at all.
+	if relativeConfigDir == "." {
+		relativeConfigDir = ""
+	}


You're computing the relative path, and then joining again at the call site. Instead of precomputing the relative path, I think it would be simpler to do that when the .envrc is generated.

mikeland73 · 2025-07-02T00:50:07Z

internal/boxcli/generate.go


 	box, err := devbox.Open(&devopt.Opts{
-		Dir:         flags.config.path,
+		Dir:         filepath.Join(envrcDir, configDir),


keep this as flags.config.path,

mikeland73 · 2025-07-02T00:50:51Z

internal/boxcli/generate.go

+
+	generateOpts := devopt.EnvrcOpts{
+		EnvrcDir:  envrcDir,
+		ConfigDir: configDir,


no need to supply ConfigDir since it is already supplied by devopt.Opts

This is also a relative path (but named ConfigDir) which I'm afraid would get really confusing later on.

no need to supply ConfigDir since it is already supplied by devopt.Opts

This is using a new type, EnvrcOpts, which does not contain ConfigDir, so I think it's needed here.

mikeland73 · 2025-07-02T00:51:20Z

internal/boxcli/generate.go

+	}
+
+	// Determine the directories for .envrc and config
+	configDir, envrcDir, err := determineDirenvDirs(flags.config.path, flags.envrcDir)


I think determineDirenvDirs can be removed and just compute the relative path internally when generating the .envrc

mikeland73 · 2025-07-02T01:02:27Z

internal/devbox/generate/devcontainer_util.go

 	// write content into file
 	return t.Execute(file, map[string]string{
 		"Flags": strings.Join(flags, " "),
+		"Dir":   opts.ConfigDir,


Name this ConfigDir to reduce ambiguity.

Compute relative path here instead of generate.go

Make empty if envrcDir is not specified.

I did change the name to ConfigDir.

mikeland73 · 2025-07-02T01:07:00Z

internal/devbox/generate/tmpl/envrcContent.tmpl

+{{define "DirPrefix"}}{{ if .Dir }}{{ .Dir }}/{{ end }}{{end}}
 use_devbox() {
-    watch_file devbox.json devbox.lock
-    eval "$(devbox shellenv --init-hook --install --no-refresh-alias{{ if .EnvFlag }} {{ .EnvFlag }}{{ end }})"
+    watch_file {{template "DirPrefix" .}}devbox.json {{template "DirPrefix" .}}devbox.lock
+    eval "$(devbox shellenv --init-hook --install --no-refresh-alias {{ if .EnvFlag }}{{ .EnvFlag }}{{ end }} {{- if .Dir }}--config {{ .Dir -}}{{ end }})"


Templates are pretty hard to read. Ideally this is computed in Go code and just passed in. e.g.

watch_file {{ .RelativeDevboxJSONPath }} {{ .RelativeDevboxLockPath }}

You could do similar with --config flag.

I cleaned this up.

pstephan-geico-external

@mikeland73, thanks for your feedback. I did incorporate a number of your comments. However, others relate to your intro comment:

--envrc-dir should not control/override --config. It should be independent.

The issue I was trying to address is that the current version incorrectly (in my opinion) uses --config to specify the location of the .envrc file when specified as part of devbox generate direnv. The change also allows a config file location to be specified that will be used with direnv and the .envrc file. The devbox generate direnv command does not use the devbox config file, but needs to know where it can be found so that it can properly configure the .envrc file (by including the config file with devbox generate direnv --print-envrc AND by including the config file with the output of that same command).

Before addressing your other comments, I wanted to respond to this concern as it may impact some of those other comments and we should probably resolve this concern first.

pstephan-geico-external · 2025-07-02T14:23:26Z

internal/boxcli/generate.go

+		"path to directory where the .envrc file should be generated. "+
+			"If not specified, the .envrc file will be generated in "+
+			"the current working directory.")
+


Good Catch and good suggestion.

pstephan-geico-external · 2025-07-07T14:15:35Z

internal/devbox/generate/devcontainer_util.go

 	// write content into file
 	return t.Execute(file, map[string]string{
 		"Flags": strings.Join(flags, " "),
+		"Dir":   opts.ConfigDir,


I did change the name to ConfigDir.

pstephan-geico-external · 2025-07-07T14:38:21Z

@mikeland73 thanks for your feedback. I did incorporate a number of your comments. However, others relate to your intro comment:

--envrc-dir should not control/override --config. It should be independent.

The issue I was trying to address is that the current version incorrectly (in my opinion) uses --config to specify the location of the .envrc file when specified as part of devbox generate direnv. The change also allows a config file location to be specified that will be used with direnv and the .envrc file. The devbox generate direnv command does not use the devbox config file, but needs to know where it can be found so that it can properly configure the .envrc file (by including the config file with devbox generate direnv --print-envrc AND by including the config file with the output of that same command).

mikeland73 · 2025-07-15T19:01:23Z

@mikeland73 thanks for your feedback. I did incorporate a number of your comments. However, others relate to your intro comment:

--envrc-dir should not control/override --config. It should be independent.

The issue I was trying to address is that the current version incorrectly (in my opinion) uses --config to specify the location of the .envrc file when specified as part of devbox generate direnv. The change also allows a config file location to be specified that will be used with direnv and the .envrc file. The devbox generate direnv command does not use the devbox config file, but needs to know where it can be found so that it can properly configure the .envrc file (by including the config file with devbox generate direnv --print-envrc AND by including the config file with the output of that same command).

TLDR:

I don't think we're that far apart. I mostly just want to simplify the logic in internal/boxcli/generate.go and just pass the envrcDir down the stack. I also don't want configDir to mean "relative path from envrc to config dir" since this is only relevant to .envrc file, but not the devbox itself. You should only compute it when generating the .envrc itself.

Longer:

I'm not sure I agree 100% that --config works incorrectly. Specifically, --config is a semi-global flag that runs a command using the context of a specific devbox.json. This means that the working directory becomes the directory where the devbox.json resides. For example:

devbox run --config examples/development/php/latest ls

Will print out the content of the directory in examples/development/php/latest.

I do think devbox generate direnv is a bit different from most devbox commands, but it's not clear to me that creating .envrc in the working directory is strictly a better option (If I did, I would be OK with just changing the behavior of the existing flag).

That said, I like your solution of adding a new flag. I think it solves the edge case without making --config behave differently for a single command. With the new flag, I would expect --config to continue behaving the same way.

Specifically:

devbox generate direnv --config path/to/my/config --envrc-dir path/to/create/envrc

Would create a new .envrc file in path/to/create/envrc and point to the devbox project in path/to/my/config

You would need to figure out the relative path between them, but this is trivial (your code already does this).

What I don't totally understand is why when both flags are passed, you change the meaning of the config flag to be relative (from the envrc). This logic is needed when printing the .envrc, but outside of that context it is incorrect. In fact, you correct for this issue by rejoining paths:

box, err := devbox.Open(&devopt.Opts{
		Dir:         filepath.Join(envrcDir, configDir),

But at this point, configDir is a misleading variable name, since it's a relative path from the envrc.

mikeland73

The main issue in this PR isdetermineDirenvDirs. It should probably not exist, because:

it redefines the meaning of configDir
The logic is a bit weird (it computes relative path only in certain cases)
nit: It returns 3 values (we rarely do this in this codebase)

Related., we do not need ConfigDir in EnvrcOpts because ConfigDir is already known by the devbox struct.

pstephan-geico-external · 2025-08-22T16:00:57Z

The main issue in this PR isdetermineDirenvDirs. It should probably not exist, because:

it redefines the meaning of configDir

The logic is a bit weird (it computes relative path only in certain cases)

nit: It returns 3 values (we rarely do this in this codebase)

Related., we do not need ConfigDir in EnvrcOpts because ConfigDir is already known by the devbox struct.

@mikeland73 sorry it has taken a little while to get back to this. After considering your thoughtful comments, we agree that the config should NOT be relative to the .envrc file. Making that change simplified some other things including eliminating any possible need for determineDirenvDirs

I think these changes simplify things while still accomplishing the goal. Note that one other change is the output from --print-envrc, which now invokes devbox shellenv... prior to the watch_file command. This allows us to use $DEVBOX_PROJECT_ROOT to specify the path to the config and lock files. This ensures the correct files are being watched.

Several additional tests were also added to verify the various combinations of things.

mikeland73 · 2025-08-29T21:23:08Z

@pstephan-geico-external thanks for addressing the issues. Sorry for delay, this is on my queue, hope to get to it pretty soon.

pstephan-geico-external · 2025-09-15T14:45:34Z

@mikeland73 - just checking on this

pstephan-geico-external · 2025-09-30T19:49:54Z

@mikeland73 - any update on this?

pstephan-geico-external · 2025-10-23T20:49:30Z

@mikeland73, I'm unable to move forward on this until I hear back from you. With the lag that has happened on this, I'm going to leave the ball in your court whether or not you want to proceed.

mikeland73

@pstephan-geico-external I'm embarrassed at how long it took me to get to this, I'm really sorry. This looks really good.

Let me do some extra smoke testing and will look to get this into a release ASAP.

Add --envrc-dir flag to allow specifying location of direnv config

415686e

darch-geico-external suggested changes Jun 11, 2025

View reviewed changes

Updates in response to code review

6da0d57

darch-geico-external suggested changes Jun 12, 2025

View reviewed changes

internal/devbox/devbox.go Outdated Show resolved Hide resolved

Corrected the allow command

df12f68

darch-geico-external approved these changes Jun 12, 2025

View reviewed changes

jguluarte approved these changes Jun 17, 2025

View reviewed changes

internal/boxcli/generate.go Outdated Show resolved Hide resolved

internal/boxcli/generate.go Show resolved Hide resolved

More review tweaks

d005a8d

LucilleH requested a review from Copilot July 1, 2025 19:12

Copilot AI reviewed Jul 1, 2025

View reviewed changes

internal/boxcli/generate.go Outdated Show resolved Hide resolved

mikeland73 self-requested a review July 1, 2025 20:39

mikeland73 requested changes Jul 2, 2025

View reviewed changes

Cleanup in response to PR comments

8b57442

pstephan-geico-external commented Jul 7, 2025

View reviewed changes

pstephan-geico-external requested a review from mikeland73 July 15, 2025 14:04

mikeland73 requested changes Jul 15, 2025

View reviewed changes

More cleanup following review; config is no longer relative to .envrc

928c344

pstephan-geico-external requested review from darch-geico-external, jguluarte and mikeland73 August 22, 2025 16:01

mikeland73 approved these changes Oct 23, 2025

View reviewed changes

mikeland73 added 2 commits October 23, 2025 14:17

Merge branch 'main' into direnv-respect-config

84a0fdb

return error if relpath can't be found

17d7b6f

Merge branch 'main' into direnv-respect-config

7f7ba14

mikeland73 merged commit 6c517a0 into jetify-com:main Oct 26, 2025
55 of 56 checks passed

Add --envrc-dir flag to allow specifying location of direnv config #2629

Add --envrc-dir flag to allow specifying location of direnv config #2629

Uh oh!

Conversation

pstephan-geico-external commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Community Contribution License

Uh oh!

darch-geico-external left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darch-geico-external left a comment

Choose a reason for hiding this comment

Uh oh!

jguluarte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

mikeland73 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstephan-geico-external left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstephan-geico-external commented Jun 11, 2025 •

edited

Loading