-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Move InstallDependencies to the language plugin #9294
Conversation
This changes `pulumi new` and `pulumi up <template>` to invoke the language plugin to install dependencies, rather than having the code to install dependencies hardcoded into the cli itself. This does not change the way policypacks or plugin dependencies are installed. In theory we can make pretty much the same change to just invoke the language plugin, but baby steps we don't need to make that change at the same time as this. We used to feed the result of these install commands (dotnet build, npm install, etc) directly through to the CLI stdout/stderr. To mostly maintain that behaviour the InstallDependencies gRCP method streams back bytes to be written to stdout/stderr, those bytes are either read from pipes or a pty that we run the install commands with. The use of a pty is controlled by the global colorisation option in the cli. An alternative designs was to use the Engine interface to Log the results of install commands. This renders very differently to just writing directly to the standard outputs and I don't think would support control codes so well. The design as is means that `npm install` for example is still able to display a progress bar and colors even though we're running it in a separate process and streaming its output back via gRPC. The only "oddity" I feel that's fallen out of this work is that InstallDependencies for python used to overwrite the virtualenv runtime option. It looks like this was because our templates don't bother setting that. Because InstallDependencies doesn't have the project file, and at any rate will be used for policy pack projects in the future, I've moved that logic into `pulumi new` when it mutates the other project file settings. I think we should at some point cleanup so the templates correctly indicate to use a venv, or maybe change python to assume a virtual env of "venv" if none is given?
pkg/cmd/pulumi/new.go
Outdated
@@ -254,6 +250,14 @@ func runNew(args newArgs) error { | |||
proj.Name = tokens.PackageName(args.name) | |||
proj.Description = &args.description | |||
proj.Template = nil | |||
// Hack for python, most of our templates don't specify a venv but we want to use one |
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.
s/Hack/Workaround/
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 this actually will break some users, like Nix users. There are situations where users explicitly don't want to use a venv. If Python now always uses venv even when not called for, we should highlight this as a change in CHANGELOG.
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.
Ah cool I was going to ask if we could just assume "venv" but that's a good reason not to. At any rate this isn't a breaking change because we already always set venv in "pulumi new"
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.
IN that case it's completely fine ofc.
@@ -189,9 +190,16 @@ func runNewPolicyPack(args newPolicyArgs) error { | |||
func installPolicyPackDependencies(proj *workspace.PolicyPackProject, projPath, root string) error { | |||
// TODO[pulumi/pulumi#1334]: move to the language plugins so we don't have to hard code here. | |||
if strings.EqualFold(proj.Runtime.Name(), "nodejs") { | |||
if bin, err := nodeInstallDependencies(); err != nil { | |||
fmt.Println("Installing dependencies...") |
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.
So we need to use logging instead of fmt.Println?
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.
So this is copied from nodeInstallDependencies which already used fmt.Println
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.
It's moving processes though right. The plugin executable runs in a process where stdout is used to communicate the port on which the engine is running. Printing after that might work. I'm just careful. Hopefully the tests cover us here.
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.
Nope this hasn't moved process
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.
Ah never mind. Perfect ty. pkg/cmd you're right.
} | ||
|
||
if len(output.Stdout) != 0 { | ||
os.Stdout.Write(output.Stdout) |
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.
Just sanity checking, langhost writes to stdout/stderr, or needs to "write" via gRPC?
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.
Yeh we use to call a subprocess and hook it's stdout to our stdout. Now we stream back stdout via grpc but we still want it to just write to stdout here.
} | ||
|
||
// Returns a pair of streams for use with the language runtimes InstallDependencies method | ||
func MakeStreams( |
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.
Interesting...
@@ -28,6 +28,9 @@ service LanguageRuntime { | |||
rpc Run(RunRequest) returns (RunResponse) {} | |||
// GetPluginInfo returns generic information about this plugin, like its version. | |||
rpc GetPluginInfo(google.protobuf.Empty) returns (PluginInfo) {} | |||
|
|||
// InstallDependencies will install dependencies for the project, e.g. by running `npm install` for nodejs projects. | |||
rpc InstallDependencies(InstallDependenciesRequest) returns (stream InstallDependenciesResponse) {} |
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.
Aha so we're extending the gRPC interface here.
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.
LGTM given the motivation around stream magic code.
We can consider editing the templates to specify venv rather than continuing to maintain magic venv assumptions.
The BW compat here assumes that CLI and language plugins upgrade/downgrade in lock step so were good with this change right? |
That would be really nice.
Yes, but we'd be ok with this even for new languages because they wouldn't of been doing anything for InstallDependencies anyway. |
Still a few errors in CI that need digging into here, and my last test on macOS didn't behave exactly as I expected (npm install didn't show a loading bar that I expected). So I'll do a bit more work on this before merging. |
* Move InstallDependencies to the language plugin This changes `pulumi new` and `pulumi up <template>` to invoke the language plugin to install dependencies, rather than having the code to install dependencies hardcoded into the cli itself. This does not change the way policypacks or plugin dependencies are installed. In theory we can make pretty much the same change to just invoke the language plugin, but baby steps we don't need to make that change at the same time as this. We used to feed the result of these install commands (dotnet build, npm install, etc) directly through to the CLI stdout/stderr. To mostly maintain that behaviour the InstallDependencies gRCP method streams back bytes to be written to stdout/stderr, those bytes are either read from pipes or a pty that we run the install commands with. The use of a pty is controlled by the global colorisation option in the cli. An alternative designs was to use the Engine interface to Log the results of install commands. This renders very differently to just writing directly to the standard outputs and I don't think would support control codes so well. The design as is means that `npm install` for example is still able to display a progress bar and colors even though we're running it in a separate process and streaming its output back via gRPC. The only "oddity" I feel that's fallen out of this work is that InstallDependencies for python used to overwrite the virtualenv runtime option. It looks like this was because our templates don't bother setting that. Because InstallDependencies doesn't have the project file, and at any rate will be used for policy pack projects in the future, I've moved that logic into `pulumi new` when it mutates the other project file settings. I think we should at some point cleanup so the templates correctly indicate to use a venv, or maybe change python to assume a virtual env of "venv" if none is given? * Just warn if pty fails to open * Add tests and return real tty files * Add to CHANGELOG * lint * format * Test strings * Log pty opening for trace debugging * s/Hack/Workaround * Use termios * Tweak terminal test * lint * Fix windows build
Description
This changes
pulumi new
andpulumi up <template>
to invoke the language plugin to install dependencies, rather than having the code to install dependencies hardcoded into the cli itself.This does not change the way policypacks or plugin dependencies are installed. In theory we can make pretty much the same change to just invoke the language plugin, but baby steps we don't need to make that change at the same time as this.
We used to feed the result of these install commands (dotnet build, npm install, etc) directly through to the CLI stdout/stderr. To mostly maintain that behaviour the InstallDependencies gRCP method streams back bytes to be written to stdout/stderr, those bytes are either read from pipes or a pty that we run the install commands with. The use of a pty is controlled by the global colorisation option in the cli.
An alternative designs was to use the Engine interface to Log the results of install commands. This renders very differently to just writing directly to the standard outputs and I don't think would support control codes so well.
The design as is means that
npm install
for example is still able to display a progress bar and colors even though we're running it in a separate process and streaming its output back via gRPC.The only "oddity" I feel that's fallen out of this work is that InstallDependencies for python used to overwrite the virtualenv runtime option. It looks like this was because our templates don't bother setting that. Because InstallDependencies doesn't have the project file, and at any rate will be used for policy pack projects in the future, I've moved that logic into
pulumi new
when it mutates the other project file settings. I think we should at some point cleanup so the templates correctly indicate to use a venv, or maybe change python to assume a virtual env of "venv" if none is given?Fixes #1334
Checklist