-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 and simplify build instructions for Windows #2612
Conversation
I am reviewing it now...
Why is it relevant? @adamretter, please sign your commit. Otherwise, we cannot accept it. |
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 did not have the chance to try it out from scratch; I have everything already installed on Windows. I never use cmd.exe
anyway, but the documentation looks good. Thank you for your help!
I have made a few recommendations, if you have questions, please ask.
doc/Developing.md
Outdated
@@ -235,33 +235,61 @@ etc.) by opening `packages/<package name>/coverage/index.html`. | |||
|
|||
## Building on Windows | |||
|
|||
Run cmd.exe as an administrator and install `choco` by copy-pasting the command | |||
to your console: | |||
1. Run cmd.exe as an Administrator. You can do this by right-clicking "Command Prompt" in the Windows Start Menu, and then right-clicking "Run as Administrator". |
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.
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.
Which version of Windows do you have? Mine looks significantly different to that...
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.
Windows 10 Pro.
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.
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 was confused. I did the right click on the Start Menu. Great, then it is OK.
By the way there is another way without clicking: https://superuser.com/questions/968214/open-cmd-as-admin-with-windowsr-shortcut
- Win+R
- Type:
cmd
. - Alt+Ctrl+Enter
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.
There is one small mistake here in the choco command for yarn. You should use the -i option to ignore dependencies or you will find that yarn will install Node 10+ as well, overriding your initial Node LTS install.
doc/Developing.md
Outdated
refreshenv | ||
``` | ||
|
||
4. Install NodeJS via. choco (**NOTE**: the version is important!) by running the following in the Command Prompt: |
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.
NodeJS
-> Node.js
doc/Developing.md
Outdated
choco install yarn --version 1.7.0 -y | ||
``` | ||
|
||
6. Install Git via. Choco: |
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.
Choco -> choco (or choco
).
Please if you escape something or put it between the double quotes, use it consistently.
"Command Prompt", Command Prompt
Choco, choco, choco
.
doc/Developing.md
Outdated
choco install git | ||
|
||
Install the correct version of `yarn` (The version is important) | ||
2. Install `choco` by copy and pasting the command below into the Command Prompt, and then pressing the `return` key to run it: |
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.
`return`
return
<kbd>return</kbd>
return
#2009 is relevant because it states that Theia does not run on Node v10.3.0. This PR ensures that Node.js 8.11.4 is used. |
b7d3de8
to
52dfab7
Compare
@kittaakos Okay, I made the changes you asked for, rebased and signed the commits. |
Awesome! What about the "Command Prompt" in the Windows Start Menu, and then right-clicking "Run as Administrator". As you can see, I do not have such an option.
You need to squash the commits and sign it. Make sure you use the same email address as your GH account has. |
I would update the instructions to include the -i flag when installing yarn from chocolatey. This tells chocolatey to not install node again. If you forget this flag, it will likely pull in Node 10+ as well. |
@kittaakos okay I fixed the run as Administrator thing and squashed and signed the commits. @dunnry I did not add the |
… be done from cmd.exe (don't need different shells) Closes eclipse-theia#1614 See also eclipse-theia#2009 Signed-off-by: Adam Retter <adam.retter@googlemail.com>
For committers/reviewers:
✅ |
@adamretter do you still plan on amending the PR in order for it to be merged? |
I'll be closing the PR for the moment due to inactivity :( |
Closes #1614
Also relevant #2009