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

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

Merged
merged 1 commit into from
Jul 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,36 @@ aws s3 cp <s3-url> ~/aws-cdk.zip

### Install to ~/.cdk

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 (bash/zsh)

```shell
# Install to ~/.cdk
rm -fr ~/.cdk
mkdir ~/.cdk
unzip <path-to-zip-file> -d ~/.cdk

# Add ~/.cdk/bin to your PATH
echo 'PATH=$PATH:$HOME/.cdk/bin' >> ~/.bashrc
echo 'PATH=$PATH:$HOME/.cdk/bin' >> ~/.zshrc
```

Make sure the `~/.cdk/bin` is in your `PATH`
#### Windows (PowerShell)

```shell
# at the end of your ~/.bashrc or ~/.zshrc file
export PATH=$PATH:$HOME/.cdk/bin
```powershell
# Install to ~/.cdk
Remove-Item -Force -Recurse ~/.cdk
New-Item -Type Directory ~/.cdk
Expand-Archive -Path <path-to-zip-file> -DestinationPath ~/.cdk

# 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"'
```

Copy link
Contributor

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

Copy link
Contributor

Choose a reason for hiding this comment

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

H3 is correct I believe

### Install the command-line toolkit and docs

Install (or update) `aws-cdk` and `aws-cdk-docs` globally

```shell
Expand Down
3 changes: 3 additions & 0 deletions bundle-beta.sh
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ done

echo "Installing y-npm" # using y-npm, we're so META!
${Y_NPM} install --global-style --no-save y-npm
# Because y-npm is installed on the build server, we need to bootstrap
# it on windows by manually creating the shim batch file.
cp ${root}/tools/y-npm/bin/y-npm.template.cmd node_modules/.bin/y-npm.cmd
ln -s node_modules/.bin bin

# Create a local maven repository
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"%name%": "bin/%name%.js"
},
"scripts": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

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

"prepare": "tsc && chmod a+x bin/%name%.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

That should be "build" now, in fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

"build": "tsc",
"watch": "tsc -w",
"cdk": "cdk"
},
Expand Down
6 changes: 4 additions & 2 deletions packages/aws-cdk/lib/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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') :
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 problem is that the Windows doesn't understand the symlink, so we have to give it a symlink-less path.

path.join(CDK_HOME, 'bin', 'y-npm');
const command = await fs.pathExists(yNpm) ? yNpm : 'npm';
print(`Executing ${colors.green(`${command} install`)}...`);
try {
Expand Down Expand Up @@ -245,7 +247,7 @@ function isRoot(dir: string) {
* @returns STDOUT (if successful).
*/
async function execute(cmd: string, ...args: string[]) {
const child = spawn(cmd, args, { stdio: [ 'ignore', 'pipe', 'inherit' ] });
const child = spawn(cmd, args, { shell: true, stdio: [ 'ignore', 'pipe', 'inherit' ] });
let stdout = '';
child.stdout.on('data', chunk => stdout += chunk.toString());
return new Promise<string>((ok, fail) => {
Expand Down
7 changes: 7 additions & 0 deletions tools/y-npm/bin/y-npm.template.cmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
@IF EXIST "%~dp0\node.exe" (
"%~dp0\node.exe" "%~dp0\..\y-npm\bin\y-npm" %*
) ELSE (
@SETLOCAL
@SET PATHEXT=%PATHEXT:;.JS;=;%
node "%~dp0\..\y-npm\bin\y-npm" %*
)
4 changes: 3 additions & 1 deletion tools/y-npm/lib/run-npm-command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ export function runCommand(command: string, args: string[], additionalEnv?: Node
env[key] = value;
}
}
const child = spawn(command, args, { detached: false, env, stdio: ['inherit', 'pipe', 'pipe'] });
// `shell: true` is required because on Windows, batch files must be run from a shell, and y-npm
// is invoked using the batch file y-npm.cmd (to work around symlink issues on Windows).
const child = spawn(command, args, { detached: false, env, shell: true, stdio: ['inherit', 'pipe', 'pipe'] });
debug(`Command PID: ${child.pid}`);
const stdout = new Array<Buffer>();
const stderr = new Array<Buffer>();
Expand Down