Fix setup issues on Windows, and update README with Windows-specific steps where required. #320

mpiroc · 2018-07-11T22:17:21Z

By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.

RomainMuller · 2018-07-12T06:31:18Z

README.md

@@ -39,19 +39,37 @@ aws s3 cp <s3-url> ~/aws-cdk.zip

 Once you've downloaded the bits, install them into `~/.cdk`:

+#### Linux/MacOS (Shell)


I think I would prefer having one block for *NIX instructions, and one block for Windows instructions. Interweaving them *NIX - Windows - *NIX - Windows... Seems not convenient.

I am not sure I agree. When we move these to our docs, we can just use tabs, and this also makes sure the steps are consistent between the platforms.

Does that not effectively separate them out as well? You better not make me click the "Windows" tab on every new section!

I'm with Romain. Think of the user experience, and how you would feel when presented with fragments of instructions that you would have to piece together.

I agree with Rico and Romain, the installation instructions for Win and *NIX should be in separate blocks of text with a clear title indicating which OS the instructions are for. They should not be interwoven together.

The tab selection is "sticky" for all sections/topics.

RomainMuller · 2018-07-12T06:32:10Z

packages/aws-cdk/lib/init-templates/app/typescript/package.template.json

@@ -7,7 +7,7 @@
        "%name%": "bin/%name%.js"
    },
    "scripts": {
-        "prepare": "tsc && chmod a+x bin/%name%.js",


That should be "build" now, in fact.

Even in the init templates? prepare is a well-known script name, but build isn't (https://docs.npmjs.com/misc/scripts).

Holding off on changing to build this iteration.

Yes, build is the idiomatic way. We've been abusing prepare (my fault).

RomainMuller · 2018-07-12T06:33:28Z

packages/aws-cdk/lib/init-templates/app/typescript/scripts/prepare.js

+child_process.spawn('tsc')
+
+if (os.platform() !== 'win32') {
+    child_process.spawn('chmod', [ 'a+x', 'bin/%name%.js' ])


This won't be turned into the actual name unless you name the file prepare.template.js

wontfix (prepare.js has been removed)

RomainMuller · 2018-07-12T06:33:53Z

packages/aws-cdk/lib/init.ts

@@ -201,7 +201,10 @@ async function postInstall(language: string) {
 }

 async function postInstallTypescript() {
-    const yNpm = path.join(CDK_HOME, 'bin', 'y-npm');
+


Why the blank line?

To keep you code reviewers on your toes! >_>

RomainMuller · 2018-07-12T06:36:25Z

packages/aws-cdk/lib/init.ts

-    const yNpm = path.join(CDK_HOME, 'bin', 'y-npm');
+
+    const yNpm = os.platform() === 'win32' ?
+        path.join(CDK_HOME, 'node_modules', '.bin', 'y-npm.cmd') :


Is the .cmd extension necessary when you pass shell: true to spawn? It seems not needed when spawning npm, so I'm slightly confused.

I'm also fairly sure it's not necessary.

.cmd isn't necessary for spawn, but it is necessary for fs.pathExists(yNpm).

eladb · 2018-07-12T07:56:35Z

README.md

@@ -39,19 +39,37 @@ aws s3 cp <s3-url> ~/aws-cdk.zip

 Once you've downloaded the bits, install them into `~/.cdk`:

+#### Linux/MacOS (Shell)


I am not sure I agree. When we move these to our docs, we can just use tabs, and this also makes sure the steps are consistent between the platforms.

eladb · 2018-07-12T07:57:33Z

tools/y-npm/bin/.y-npm.cmd

@@ -0,0 +1,7 @@
+@IF EXIST "%~dp0\node.exe" (


Why is does this file name start with a "."?

I wanted to avoid any confusion between %~dp0\..\y-npm\bin\y-npm and %~dp0\..\y-npm\bin\.y-npm.cmd.

I will rename from .y-npm.cmd to y-npm.template.cmd.

eladb · 2018-07-12T07:58:08Z

tools/y-npm/bin/.y-npm.cmd

@@ -0,0 +1,7 @@
+@IF EXIST "%~dp0\node.exe" (
+  "%~dp0\node.exe"  "%~dp0\..\y-npm\bin\y-npm" %*


eladb · 2018-07-12T07:58:27Z

tools/y-npm/lib/run-npm-command.ts

@@ -80,7 +80,7 @@ export function runCommand(command: string, args: string[], additionalEnv?: Node
                    env[key] = value;
                }
            }
-            const child = spawn(command, args, { detached: false, env, stdio: ['inherit', 'pipe', 'pipe'] });
+            const child = spawn(command, args, { detached: false, env, shell: true, stdio: ['inherit', 'pipe', 'pipe'] });


Add a comment explaining why shell: true is needed

eladb · 2018-07-12T08:00:26Z

packages/aws-cdk/lib/init-templates/app/typescript/package.template.json

@@ -7,7 +7,7 @@
        "%name%": "bin/%name%.js"
    },
    "scripts": {


This is messy.

I rather we do this:

Instead of bin/xxx.ts, let's go back to using lib/index.ts as the entrypoint of apps and have cdk.json run node lib/index.js. Much simpler, cross platform and less messy. This will make it also easier to migrate to #216.

Surely the place where the entry-point file lives (bin/ vs lib/) has no bearing on how it's executed? I vote we still put the entry point in bin/main.ts so it's very clear and easy to find, and everything in lib/ can automatically be assumed to be application-specific constructs.

Then, we can still decide on how we want to execute it, which can still be through node bin/main.js.

Fair enough. Let's just not make it executable then

✔️

Removed chmod invocation, and remove prepare.js as it is no longer needed without the chmod invocation.

eladb · 2018-07-16T07:23:51Z

README.md

-Once you've downloaded the bits, install them into `~/.cdk`:
+Once you've downloaded the bits, install them into `~/.cdk` and make sure that `~/.cdk/bin` is in your `PATH`:
+
+#### Linux/MacOS (Shell)


Change "Shell" to "bash/zsh"

Will do. I couldn't find a profile that is read by all interactive shells (.profile is only read by login shells). If I missed one let me know--writing to a single profile would be better than writing to profiles for all common shells.

eladb · 2018-07-16T07:26:01Z

README.md

+
+# Add ~/.cdk/bin to your PATH
+$profilePath = Join-Path ([Environment]::GetFolderPath([Environment+SpecialFolder]::MyDocuments)) "Profile.ps1"
+Add-Content -Path $profilePath -Value '$env:Path = "$env:Path;$env:UserProfile\.cdk\bin"'
 ```



There should be an H2 (##) here which separates "Install to ~/.cdk" section and the next section which is "Install the command-line toolkit and docs"

eladb · 2018-07-16T07:28:10Z

README.md

+
+# Add ~/.cdk/bin to your PATH
+$profilePath = Join-Path ([Environment]::GetFolderPath([Environment+SpecialFolder]::MyDocuments)) "Profile.ps1"
+Add-Content -Path $profilePath -Value '$env:Path = "$env:Path;$env:UserProfile\.cdk\bin"'
 ```



There should be an H3 (###) here to separate the "Install to ~/.cdk" section and the next section which is "Install the command-line toolkit and docs".

I see two PR comments (one suggesting H2, the other suggesting H3). I will add an H3 header.

H3 is correct I believe

eladb · 2018-07-19T05:43:08Z

@RomainMuller any more comments? Can you approve the change?

RomainMuller · 2018-07-19T07:12:45Z

packages/aws-cdk/lib/init.ts

@@ -201,7 +201,9 @@ async function postInstall(language: string) {
 }

 async function postInstallTypescript() {
-    const yNpm = path.join(CDK_HOME, 'bin', 'y-npm');
+    const yNpm = os.platform() === 'win32' ?
+        path.join(CDK_HOME, 'node_modules', '.bin', 'y-npm.cmd') :


In principle, ${CDK_HOME}/bin is a symlink to node_modules/.bin, so I'm not sure the distinction is 100% useful here. It doesn't hurt though.

The problem is that the Windows doesn't understand the symlink, so we have to give it a symlink-less path.

RomainMuller suggested changes Jul 12, 2018

View reviewed changes

eladb suggested changes Jul 12, 2018

View reviewed changes

mpiroc force-pushed the pirocchi/win branch from c269d8a to 9696432 Compare July 13, 2018 20:56

eladb approved these changes Jul 16, 2018

View reviewed changes

mpiroc force-pushed the pirocchi/win branch from 4ecc914 to f2df954 Compare July 17, 2018 23:14

Fix setup on Windows and update README for Windows.

7124ff1

mpiroc force-pushed the pirocchi/win branch from f2df954 to 7124ff1 Compare July 18, 2018 18:21

RomainMuller approved these changes Jul 19, 2018

View reviewed changes

mpiroc merged commit 65f4a9b into master Jul 19, 2018

mpiroc deleted the pirocchi/win branch July 19, 2018 20:09

		@@ -39,19 +39,37 @@ aws s3 cp <s3-url> ~/aws-cdk.zip

		Once you've downloaded the bits, install them into `~/.cdk`:

		#### Linux/MacOS (Shell)

		@@ -0,0 +1,7 @@
		@IF EXIST "%~dp0\node.exe" (
		"%~dp0\node.exe" "%~dp0\..\y-npm\bin\y-npm" %*

Fix setup issues on Windows, and update README with Windows-specific steps where required. #320

Fix setup issues on Windows, and update README with Windows-specific steps where required. #320

Conversation

mpiroc commented Jul 11, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment