Skip to content

make the env.sh script idempotent #8644

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

Closed
wants to merge 1 commit into from

Conversation

nebkor
Copy link

@nebkor nebkor commented Jul 18, 2025

Hello! I noticed that the dev env setup script would keep pre-pending the omicron paths to $PATH; this just removes the duplicates (bonus of removing all dupes from your $PATH) while preserving order; works with zsh and bash.

Before, after sourcing the env script twice:

$ echo $PATH |tr ':' '\n'
/home/ardent/git/omicron/out/mgd/root/opt/oxide/mgd/bin
/home/ardent/git/omicron/out/dendrite-stub/bin
/home/ardent/git/omicron/out/clickhouse
/home/ardent/git/omicron/out/cockroachdb/bin
/home/ardent/git/omicron/out/mgd/root/opt/oxide/mgd/bin
/home/ardent/git/omicron/out/dendrite-stub/bin
/home/ardent/git/omicron/out/clickhouse
/home/ardent/git/omicron/out/cockroachdb/bin
/home/ardent/bin
....

After:

$ source env.sh ; source env.sh
......
$ echo $PATH |tr ':' '\n'
/home/ardent/git/omicron/out/mgd/root/opt/oxide/mgd/bin
/home/ardent/git/omicron/out/dendrite-stub/bin
/home/ardent/git/omicron/out/clickhouse
/home/ardent/git/omicron/out/cockroachdb/bin
/home/ardent/bin
....

@davepacheco
Copy link
Collaborator

Thanks for putting this together. While I can see the appeal of making this avoid adding to PATH every time you run it, I don't really like this change. That seemed to be the consensus when I asked around internally, too, so I'm going to close this (though if others at Oxide want to push for a change here, please go ahead and make it).

To give a little more detail, these are the kinds of things that make me nervous about a change like this: the impact of getting it wrong is pretty high (mucking with people's paths can lead to very confusing bugs and even security issues); this one-liner is not super self-evident and bash isn't a great environment for automated testing; this sort of problem has lots of edge cases (e.g., paths with spaces, or multiple paths that differ only by case); and this one-liner relies on several different commands existing and accepting the same flags and producing the same output on all the systems we care about.

@nebkor
Copy link
Author

nebkor commented Jul 28, 2025

No worries; thanks for taking a look!

@nebkor nebkor deleted the env-setup-path-dedupe branch July 28, 2025 20:31
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.

2 participants