Skip to content
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

Invoke node directly from the language host #1300

Merged
merged 1 commit into from
May 2, 2018

Conversation

ellismg
Copy link
Contributor

@ellismg ellismg commented May 1, 2018

Instead of using a shell script to jump from the language host into
node, just invoke node directly. This makes our start-up path a little
simpler to understand and indirectly fixes pulumi/home#156, where we
would fail on Windows if the -exec script was in a folder that had
spaces in it (due to a subtle interaction between how go launches cmd
files and how cmd.exe parses arguments).

Instead of using a shell script to jump from the language host into
node, just invoke node directly. This makes our start-up path a little
simpler to understand and indirectly fixes pulumi/home#156, where we
would fail on Windows if the `-exec` script was in a folder that had
spaces in it (due to a subtle interaction between how go launches cmd
files and how cmd.exe parses arguments).
@ellismg
Copy link
Contributor Author

ellismg commented May 1, 2018

@swgillespie Can you tell me what you think of this approach? I'm biased towards removing the shell scripts because they make the process of actually launching the language even harder to follow (and causes issues on windows because of how we have to invoke cmd).

@swgillespie
Copy link
Contributor

@ellismg 👍 to the approach! Definitely easier to follow what's going on without the bash and cmd. I've got a headache at the moment so I'll look at the code in more detail in a little bit.

<Exec Command="aws s3 cp s3://eng.pulumi.com/nativeruntime/windows/nativeruntime.node $(NodeJSSdkDirectory)\prebuilt\nativeruntime.node" />
<Exec Command="aws s3 cp s3://eng.pulumi.com/nativeruntime/windows/nativeruntime-v0.11.0.node $(NodeJSSdkDirectory)\prebuilt\nativeruntime-v0.11.0.node" />
<Exec Command="aws s3 cp s3://eng.pulumi.com/nativeruntime/windows/pulumi-language-nodejs-node.exe $(NodeJSSdkDirectory)\prebuilt\pulumi-language-nodejs-node.exe" />
<Exec Command="aws s3 cp s3://eng.pulumi.com/nativeruntime/windows/nativeruntime.node &quot;$(NodeJSSdkDirectory)\prebuilt\nativeruntime.node&quot;" />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of &quot; sorts of changes are to deal with the self inflected pain I face by having my user name on windows have a space in it, so my GOPATH ends up with a space in it as well.

Copy link
Contributor

@swgillespie swgillespie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

nodeExecSuffix = "-exec" // the exec shim for Pulumi to run Node programs.
// The path to the "run" program which will spawn the rest of the language host. This may be overriden with
// PULUMI_LANGUAGE_NODEJS_RUN_PATH, which we do in some testing cases.
defaultRunPath = "./node_modules/@pulumi/pulumi/cmd/run"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to worry about path separator differences? Not sure if Go can handle that gracefully or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node ends up handling this without issue. It was even okay with NODE_PATH having a mix of / and \ path separators (I validated that require('nativeruntime') worked, on windows, even when there was a space in the path and it had mixed separators)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

impressive!

@ellismg ellismg merged commit 409477b into master May 2, 2018
@lindydonna lindydonna added impact/changelog kind/bug Some behavior is incorrect or out of spec labels May 4, 2018
@pgavlin pgavlin deleted the ellismg/launch-node-directly branch June 12, 2018 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants