Move InstallDependencies to the language plugin #9294

Frassle · 2022-03-25T13:25:02Z

Description

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?

Fixes #1334

Checklist

I have added tests that prove my fix is effective or that my feature works

I have updated the CHANGELOG-PENDING file with my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

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?

sdk/go/common/util/rpcutil/writer.go

…cies

t0yv0 · 2022-03-28T13:56:24Z

pkg/cmd/pulumi/new.go

@@ -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


s/Hack/Workaround/

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.

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"

IN that case it's completely fine ofc.

t0yv0 · 2022-03-28T13:58:39Z

pkg/cmd/pulumi/policy_new.go

@@ -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...")


So we need to use logging instead of fmt.Println?

So this is copied from nodeInstallDependencies which already used fmt.Println

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.

Nope this hasn't moved process

Ah never mind. Perfect ty. pkg/cmd you're right.

t0yv0 · 2022-03-28T14:01:40Z

sdk/go/common/resource/plugin/langruntime_plugin.go

+		}
+
+		if len(output.Stdout) != 0 {
+			os.Stdout.Write(output.Stdout)


Just sanity checking, langhost writes to stdout/stderr, or needs to "write" via gRPC?

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.

t0yv0 · 2022-03-28T14:05:05Z

sdk/go/common/util/rpcutil/writer.go

+}
+
+// Returns a pair of streams for use with the language runtimes InstallDependencies method
+func MakeStreams(


Interesting...

t0yv0 · 2022-03-28T14:06:53Z

sdk/proto/language.proto

@@ -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) {}


Aha so we're extending the gRPC interface here.

t0yv0

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.

t0yv0 · 2022-03-28T14:11:16Z

The BW compat here assumes that CLI and language plugins upgrade/downgrade in lock step so were good with this change right?

Frassle · 2022-03-28T14:26:15Z

We can consider editing the templates to specify venv rather than continuing to maintain magic venv assumptions.

That would be really nice.

The BW compat here assumes that CLI and language plugins upgrade/downgrade in lock step so were good with this change right?

Yes, but we'd be ok with this even for new languages because they wouldn't of been doing anything for InstallDependencies anyway.

Frassle · 2022-03-28T14:27:44Z

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.

…cies

* 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

Frassle requested review from justinvp and t0yv0 March 25, 2022 13:25

Just warn if pty fails to open

c7cd867

Frassle commented Mar 25, 2022

View reviewed changes

sdk/go/common/util/rpcutil/writer.go Outdated Show resolved Hide resolved

Frassle added 7 commits March 25, 2022 22:03

Add tests and return real tty files

d98f8b7

Merge remote-tracking branch 'origin/master' into moveInstallDependen…

4de3e1a

…cies

Add to CHANGELOG

b25f999

lint

9e6e620

format

45cc65e

Merge remote-tracking branch 'origin/master' into moveInstallDependen…

00e51a3

…cies

Test strings

6fa09ff

t0yv0 reviewed Mar 28, 2022

View reviewed changes

t0yv0 self-requested a review March 28, 2022 14:09

t0yv0 approved these changes Mar 28, 2022

View reviewed changes

Frassle added 7 commits April 2, 2022 18:42

Merge remote-tracking branch 'origin/master' into moveInstallDependen…

ec18569

…cies

Log pty opening for trace debugging

74e477b

s/Hack/Workaround

e54302c

Use termios

dfbdb8c

Tweak terminal test

bf29a3c

lint

75ad940

Fix windows build

86aa170

Frassle merged commit 1d215c2 into master Apr 3, 2022

pulumi-bot deleted the fraser/moveInstallDependencies branch April 3, 2022 14:55

This was referenced Apr 19, 2022

Fix CI out of memory conditions on Windows pulumi/templates#263

Merged

pulumi new leaves orphan processes with open files #9437

Closed

justinvp mentioned this pull request Sep 15, 2022

Add pulumi package with subcommands gen-sdk and get-schema #10711

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move InstallDependencies to the language plugin #9294

Move InstallDependencies to the language plugin #9294

Frassle commented Mar 25, 2022 •

edited

t0yv0 Mar 28, 2022

t0yv0 Mar 28, 2022

Frassle Mar 28, 2022

t0yv0 Mar 28, 2022

t0yv0 Mar 28, 2022

Frassle Mar 28, 2022

t0yv0 Mar 28, 2022

Frassle Mar 28, 2022

t0yv0 Mar 28, 2022

t0yv0 Mar 28, 2022

Frassle Mar 28, 2022

t0yv0 Mar 28, 2022

t0yv0 Mar 28, 2022

t0yv0 left a comment

t0yv0 commented Mar 28, 2022

Frassle commented Mar 28, 2022

Frassle commented Mar 28, 2022

Move InstallDependencies to the language plugin #9294

Move InstallDependencies to the language plugin #9294

Conversation

Frassle commented Mar 25, 2022 • edited

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 left a comment

Choose a reason for hiding this comment

t0yv0 commented Mar 28, 2022

Frassle commented Mar 28, 2022

Frassle commented Mar 28, 2022

Frassle commented Mar 25, 2022 •

edited