-
Notifications
You must be signed in to change notification settings - Fork 91
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
add install-replit-nix-system-dependencies subcommand #135
Conversation
673f8e0
to
2ca030e
Compare
internal/nix/nix.go
Outdated
} | ||
|
||
func RunNixEditorCmds(cmds [][]string) { | ||
for _, cmd := range cmds { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nix-editor supports commands over stdin right? can we make this a single shell-out and pass the commands over stdin instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I view that nix-editor interface to be overkill and sort of want to kill it. If we find performance issues with this, happy to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think that interface is perfect in this scenario. ReplitNixAddToNixEditorCmds
has to be careful to avoid a race condition (if you executed nix-editor in that function in a goroutine you'd have a race-condition). having an IPC interface is perfect for repetitive execution of very-small actions, and we'll be able to add optimizations later wrt OT
e6b1309
to
00b83fd
Compare
07f7ad3
to
36154b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, do you mind writing a test :P
Why === * When installing Python packages, we want to add system dependencies to replit.nix, so the packages work. * This can eventually be expanded to other languages too. What changed === * Introduce InstallReplitNixSystemDependencies subcommand. The subcommand uses the language specfile and packages sent on the commandline (the new ones we're adding) to determine what it should try to look up in the system dependency map. * Make a default function that does nothing for all languages except Python * For Python, add a python_map.json that maps package names to system dependency install commands and embed in go * Add tests for functions that translate the map into nix-editor commands Test plan === * Remove replit.nix so we have a blank slate to start * Make a pyproject.toml file like ``` [tool.poetry.dependencies] python = ">=3.10.0,<3.11" psycopg2 = "^1.26.0" ``` * run `REPL_HOME=. go run ./cmd/upm install-replit-nix-system-dependencies` * Confirm the replit.nix file has a postgres dep in the deps and and python ld library path. * run `REPL_HOME=. go run ./cmd/upm install-replit-nix-system-dependencies pycairo` * Confirm that cairo and pkg-config are added to deps
Co-authored-by: Connor Brewster <[email protected]>
36154b0
to
499a92d
Compare
Yeah, we have a unit test here, but I'll add an integration test in a follow on. |
Why
What changed
Test plan
REPL_HOME=. go run ./cmd/upm install-replit-nix-system-dependencies
REPL_HOME=. go run ./cmd/upm install-replit-nix-system-dependencies pycairo