Build: fixes for making Calypso run on Windows under MSYS2 #2124

klimeryk · 2016-01-05T18:59:51Z

I've seen some issues (#592, #1193, #2029, #2066) about running Calypso on Windows, so since I had some experience with multi-OS makefiles from my previous job, I figured I'd give this one a stab. Turns out it wasn't that hard, especially when you just look at the diff 😛
The main pain point is that Node.js does not support building under Cygwin or MinGW (believe me, I've tried...), so we are stuck with a Windows version (MSYS2 comes with nodejs version 0.14 or something, so too old for our purposes). Which uses Windows-style paths. On the other hand, we need make - the only fresh (not 3.81 from gnuwin32 that is almost 10 years old...) one is bundled with MSYS2 or Cygwin (or babun, etc.). I've opted with MSYS2, since it's more lightweight than Cygwin. But the problem is that make and co. uses unix-style paths. Furthermore, nodejs uses git when fetching some dependencies, so we need to use the Windows build of git, not the one that can be installed in MSYS2. Argh.

Anyway, apart from these slight annoyances in the beginning, the rest is smooth sailing and everything seem to work. But that's just my initial testing, on my machine (Windows 7 64-bit). I'd love more people to test this PR, especially on Windows 8 or 10 (or whatever the cool kids use these days).

klimeryk · 2016-01-05T20:06:34Z

server/bundler/utils.js

@@ -9,7 +9,7 @@ function getAssets( stats ) {
 			name: chunk.names[0],
 			hash: chunk.hash,
 			file: filename,
-			url: path.resolve( stats.publicPath, filename )
+			url: stats.publicPath + filename,


Just a note, in case someone wonders why this change - the old version is incorrect. path.resolve should be used for real paths. But here stats.publicPath is /calypso/, a URL prefix for assets. This miraculously worked on *nix systems because it looks like a Unix path, but on Windows this invocation resulted in a path like C:\\calypso\\bundle-hash.js which is definitely not what we want :)
Simple string concatenation is fine in this case.

blowery · 2016-01-05T20:38:44Z

Makefile

@@ -78,7 +88,7 @@ run: welcome githooks-commit install build
 	@$(NODE) build/bundle-$(CALYPSO_ENV).js

 node-version:
-	@NPM_GLOBAL_ROOT=$(shell $(NPM) -g root) $(BIN)/check-node-version
+	@$(BIN)/check-node-version


This has to be the old way to pick up semver from the global install of npm /cc @aduth

Another interesting tidbit - semver is not installed (globally, or locally, of course) on Windows as part of a standard nodejs install. So you need to explicitly install it (it's in the instructions I've written as part of this PR). I've seen a discussion about how to pull in this dependency, but nothing better than the current approach struck my fancy. And since semver is available (always?) on Linux/OS X, it's not that important to address this issue. Especially, when the error message (missing module semver) is a standard one and even if the user did not RTFM, he/she should be able to figure out that semver has to be installed.
...or have I missed a better way to do this? :D

~~It's not beautiful but it works. We can always refine that later.~~ edit: I found that semver is installed on Windows as well. See d835387.

semver should not be available by default on any OS (it's an external package), but thankfully there is no reason for using it. The node version is available through process.version

thankfully there is no reason for using it. The node version is available through process.version

semver is used by bin/check-node-version for checking that the current Node version satisfies what we've defined as the minimum requirement as engines.node in package.json, prompting the developer to upgrade if they're using an earlier version.

semver should not be available by default on any OS (it's an external package)

It's true that it's not a built-in package, but as a workaround to needing to npm install semver before checking the current version, we assume that the global npm package defines it as a dependency and call semver from its node_modules directory.

If this is enough of a problem, we could probably change this to:

node-version: @$(NPM) install semver && $(BIN)/check-node-version

... and remove the shenanigans around SEMVER_GLOBAL_PATH

Right, that's what I mean: check-node-version doesn't really need semver, it is run using node already, and so has access to node's universally accessible process.version (without any requires). The package.json also already says which engine it wants at minimum, so the version check seems... superfluous: npm install should already flag that things aren't going to work if there's a node mismatch, and a custom bin script seems odd. If anything, you'd want a check on the string that CLI call node -v returns.

Semvers (actual semvers, not semver match patterns) are at least for the forseeable future string-compare safe (at least until we hit node 10.x.x), so if the node -v CLI output , or process.version value during a script run, is greater than or equal to "v4.3.0", splendid: job done, barely any code needed, and once node 10.x shows up, you can update that check to cover the new, more complicated case. That's not any time soon though.

But, if you want semver convenience it needs to go in the dependencies or devDependencies section of package.json, not be run as a makefile command. This is done automatically for you as project maintainer if you use npm install semver --save or npm install semver --save-dev, which will update the package.json for you, so you can commit that (I'd recommend --save-dev in this case).

Loading semver this way is unorthodox, yes, but it is well-explained in the comments and doesn't seem to be causing any problems. A PR to do a simple version compare would be fine, but we shouldn't use string comparison for this, at the very least we'd want to get rid of the leading v and compare each component between .s. Adding in the extra code to do this doesn't seem like a clear win to me over the current approach.

The goal of this check is to quickly execute at the beginning of the build process and abort if continuing is a waste of time. This should be done before npm install since that takes a while due to all of our dependencies. And as far as npm install flagging based on the engines.node property...

Note that, unless the user has set the engine-strict config flag, this field is advisory only. --npm docs

(Somewhat ironically, the make node-version check uses process.exitCode from Node 0.12.x, so it doesn't actually abort the build for older Node versions. PR to fix this at #3581.)

it is, though, so far four people I work with ran into the semver require call failing due to it not having been installed, with the bin/check-node-version.js script consequently crashing their attempts to run calypso, requiring manual intervention before things worked (both on windows and OSX. No linux/unix users in that particular group).

At this point in time, it seems far more sensible to just tell people "this project needs node 4+" and just assume they know how to follow the steps to install (with perhaps a mention that there is nvm for when they need multiple node versions for different projects) than to use this script. Most people are now on 5.6, with 4.3 for production servers that need the LTS reliability (0.12 falls out of LTS in April).

But if you want to keep it, this could probably be done with a preinstall hook, using "scripts": { ..., "preinstall": "npm install semver && node bin/check-node-version", ...} (given that && is cross-platform default-terminal compatible; windows's "cmd" deals with && quite well. Powershell not so much but then that's a very different kind of shell) so that a failure there can just call process.exit(1) and abort the npm install before it gets going. That way the user only needs one command, and it'll just do the right thing.

I see now that you created an issue for this (#3048) - let's take the conversation over there.

pmualaba · 2016-01-06T16:05:37Z

This patch works for the calypso login page. Once i login to wordpress.com though, the page is blank with console error: Uncaught TypeError: userUtils.filterUserObject is not a function(anonymous function) @ user.js:128(anonymous function) @ send-request.js:78onload @ index.js:142resolve @ index.js:442onmessage @ index.js:402. I don't have this error on a Mac...

dmsnell · 2016-01-07T18:28:34Z

cc: @alexdresko

alexdresko · 2016-01-07T18:33:19Z

Oh, rad! Thanks for keeping me in the loop, @dmsnell. Totes excited!

Viper007Bond · 2016-01-07T23:04:31Z

Pinging the people who previously participated in tickets about Windows support:

@localpcguy @austinsdev @limweb @HammyHavoc @DigitalDavo @hughc

klimeryk · 2016-01-08T01:28:24Z

@pmualaba: thanks for testing! However, I can't seem to reproduce the error you've reported. I've logged out, visited http://calypso.localhost:3000/, got redirected to http://calypso.localhost:3000/devdocs/start, logged in with WordPress.com, got redirected to http://calypso.localhost:3000/devdocs/welcome and could use everything as usual. No errors in the console.
Can you reproduce the bug in your setup and share the steps needed to do so?

To my fellow Automatticians: thanks for pinging the interested parties 👍

localpcguy · 2016-01-18T00:02:28Z

Thanks for the efforts on this @klimeryk. I was able to get it running using your directions on a Windows 10 PC.

Guessing there isn't any way, without significant changes like switching build tools, for this to run on the default Windows console (cmd.exe)? Even if we considered extensions to cmd (like GoW, GNU on Windows)? MSYS doesn't intimidate me, but as I've said in other threads, it could inhibit the average WordPress developer who's currently running WordPress under WAMP or similar and doesn't really use any build tools. This is assuming that there is a desire to eventually roll this out to be run by anyone installing an open-source WordPress site, not just WordPress.com sites.

klimeryk · 2016-01-18T10:31:23Z

Thank you @localpcguy for testing!
I agree, that it'd be best to get it working without the involvement of MSYS (or Cygwin and the like), just plain cmd (however plain and cumbersome it is ;)). The problem is make - I can't find an up to date version of it that runs natively on Windows. The only one I found is the gnuwin32 and GNU on Windows, but both of them are 3.81, which is over 9 years old at this moment! I think we don't use any of the newer features of make, but I don't want the Windows build to be holding us back if ever needed those.

liquidboy · 2016-01-20T17:42:11Z

just tried pulling the 'add/windows-support' branch ... followed the 'docs\windows.md' instructions ...
and when I get to the point of 'make run' in a msys console I get the following error :

$ make run
             _
    ___ __ _| |_   _ _ __  ___  ___
   / __/ _` | | | | | '_ \/ __|/ _ \
  | (_| (_| | | |_| | |_) \__ \ (_) |
   \___\__,_|_|\__, | .__/|___/\___/
               |___/|_|

Makefile:91: recipe for target 'node-version' failed
make: *** [node-version] Error 127

My Windows 10 version is (Win10 pro insider preview, 11099.rs1_release.160109-1156)

klimeryk · 2016-01-20T18:28:00Z

Thank you for testing, @liquidboy!

Are you sure node is available?
What does node --version print?
Have you installed semver as per the instructions?

liquidboy · 2016-01-20T18:48:48Z

actually @klimeryk I know what was wrong, a few weeks ago I installed Windows IoT developer kit which installed Node.js (chakra) v 0.12.7-5 ...

So my system , in program files (x86) has both

Node.js\ (v 4.2.4)
Node.js (chakra)\ (v0.12.7-5)

I solved my problem by temporarily uninstalling Node.js (chakra) .. ill re-install it when I need it (when I do IoT work)..

Worked flaulessly after that ... thanks again for this effort !

klimeryk · 2016-01-20T18:49:32Z

Great to hear that! 👍

ebinnion · 2016-02-05T23:16:03Z

docs/windows.md

+
+    ```
+    which git
+	/c/Git/cmd/git


When I install Git from the link above, and select the following values, my path looks like /c/Program Files/Git/cmd/git.

Prompt selections:

Did I miss a step when installing Git for Windows? And if so, how can we make that more clear in the doc?

The first step says:

When installing, it's best not to use folders with spaces in them.

But you are right that it might need more emphasis. The question is - did it render your install of Git useless? That is, can you use Git for Windows in the MSYS console?

nylen · 2016-02-06T08:34:06Z

This is a really complicated setup. In fact it would be much easier to just install a Linux VM and run Calypso from there. But people have asked for it, and you've done a good job finding a combination of software that works pretty well.

I made some tweaks to the documentation in 3a60c01 including screenshots and more information about the optional dependencies that could (will?) fail to install. I think it's really important to be as clear as possible since there are a lot of potential problems.

In d835387 I was also able to remove the requirement to install semver beforehand. There is a version of semver bundled with the Node.js Windows install, it's just in a different place next to the Node executables. The fact that npm is set up differently by OS is pretty non-intuitive, so I added some documentation in the comments.

After my update, make node-version passes node bin/check-node-version a MSYS path like /c/nodejs/node_modules/.... But MSYS is smart enough to realize that node is a pure Windows executable, and converts the path to a Windows path:

James@Virtualbox MSYS ~/wp-calypso
$ X=/c/git/ node -e 'console.log( process.env.X );'
C:/git/

This is also tricky and non-intuitive - perhaps it would be better to write check-node-version in a way that it can handle an MSYS path?

One other thing I noticed was a Windows Firewall warning for node. Worth documenting somewhere?

- Separate section for each OS group in install docs - Wording improvements - Clearer install instructions with screenshots - More detail about optional native dependencies of socket.io

Also add some documentation in comments.

ebinnion · 2016-02-07T01:11:44Z

Thank you for all of the work on this @klimeryk and the updated documentation @nylen. Following the new documentation, I was able to successfully get this branch running on my base model Surface 3 (although it did take what seemed like forever to build).

I can also confirm that I saw the firewall warning about Node JS.

klimeryk · 2016-02-07T15:59:45Z

I made some tweaks to the documentation in 3a60c01 including screenshots and more information about the optional dependencies that could (will?) fail to install. I think it's really important to be as clear as possible since there are a lot of potential problems.

Awesome! Especially the screenshots are a really good idea 👍

In d835387 I was also able to remove the requirement to install semver beforehand. There is a version of semver bundled with the Node.js Windows install, it's just in a different place next to the Node executables. The fact that npm is set up differently by OS is pretty non-intuitive, so I added some documentation in the comments.

Very nice! I don't know how I've missed that - maybe I got confused, because, as you pointed out, on Windows it's in a different place. Anyway - one less step during the install!

This is also tricky and non-intuitive - perhaps it would be better to write check-node-version in a way that it can handle an MSYS path?

First of all - I didn't know about the magic MSYS does when handling paths and native Windows executables. The More You Know ™
But I don't understand what's tricky and non-intuitive? The script is handed an absolute path to the semver package - the path is correct because of MSYS magic. Is the "magic" part that's tricky and non-intuitive?

I think a better solution is to make client/lib/user/shared-utils.js something like this:
module.exports = require( 'server/user-bootstrap/shared-utils' );

Great idea - I haven't thought of that! I've committed a patch with this approach in 6d40511, but I had to do it the other way around. So it's server/user-bootstrap/shared-utils.js that is exporting client/lib/user/shared-utils.js. Because, as I understand it, the server packages have access to client packages, but not the other way around. And I've tested both setups - and only the second one (server referencing client) worked.

About the firewall warning - I think we can omit that in the docs. It highly depends on the setup - if someone has installed a custom firewall, this warning won't be shown (probably the custom firewall will complain instead). And it's a standard Windows prompt - one any user (especially developer) should be familiar with. Nothing unexpected too - node.js is a requirement of Calypso, it's used for package management, so it's natural that it's asking for permission to access the internet.

Thanks you so much @ebinnion and @nylen for testing and improving this patch. Let me know if there's anything else that can be done.

PS: I'll be sure to squash all these commits into one before merging :)

klimeryk · 2016-02-07T16:01:02Z

Makefile

+# On Windows, the npm global install path is under AppData:
+# http://stackoverflow.com/a/26894197
+# `semver` is in the base `node_modules` folder for the `node` installation.
+export SEMVER_GLOBAL_PATH := $(dir $(shell which $(NODE)))/node_modules/npm/node_modules/semver


Good call on the variable name (and adding semver to it) - now it's more precise.

klimeryk · 2016-02-07T16:35:42Z

server/user-bootstrap/shared-utils.js

-	}
-
-};
+export * from 'client/lib/user/shared-utils';


I know that omitting client would also work here (lib/user/shared-utils), but I wanted to make it extra clear what we are doing here ("importing" these shared utils from the client).

nylen · 2016-02-10T22:18:11Z

Tested this out again, including the bits where framework was modified, and looks good to me.

You'll want to test logging in and out on staging after merging this, since the wpcom_user_bootstrap config key is disabled in development.

Build: fixes for making Calypso run on Windows under MSYS2

klimeryk · 2016-02-11T10:20:49Z

Deployed - seems nothing exploded or even caught on fire 😉 I did however forget to squash the commits before pushing to master... 😣
Sincere thanks to everyone involved, especially @nylen and @ebinnion for the last testing and fixes! 🙇

A couple of typo fixes. Also switch to `https` for git URL - previously: - Automattic/wp-calypso#2124 (comment) - Automattic/wp-calypso#2278

klimeryk added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress Build labels Jan 5, 2016

klimeryk self-assigned this Jan 5, 2016

klimeryk force-pushed the add/windows-support branch 5 times, most recently from f2819a8 to 4eeff39 Compare January 5, 2016 20:03

klimeryk reviewed Jan 5, 2016
View reviewed changes

klimeryk added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jan 5, 2016

blowery reviewed Jan 5, 2016
View reviewed changes

klimeryk mentioned this pull request Jan 13, 2016

i18n: switches back to using 'echo' instead of 'printf' #2370

Closed

klimeryk force-pushed the add/windows-support branch from 4eeff39 to ab8bf10 Compare January 17, 2016 17:04

klimeryk mentioned this pull request Jan 25, 2016

How Can I install this on windows7? #2737

Closed

ebinnion reviewed Feb 5, 2016
View reviewed changes

Build: fixes for making Calypso run on Windows under MSYS2

722ef2a

klimeryk force-pushed the add/windows-support branch 3 times, most recently from 40f693a to 722ef2a Compare February 6, 2016 01:04

nylen force-pushed the add/windows-support branch 2 times, most recently from d819920 to 1d81535 Compare February 6, 2016 08:40

nylen added 2 commits February 6, 2016 02:52

Reformat docs

3a60c01

- Separate section for each OS group in install docs - Wording improvements - Clearer install instructions with screenshots - More detail about optional native dependencies of socket.io

Remove the step to globally install semver

d835387

Also add some documentation in comments.

nylen force-pushed the add/windows-support branch from 1d81535 to d835387 Compare February 6, 2016 08:53

klimeryk reviewed Feb 7, 2016
View reviewed changes

Framework: export shared-utils instead of symlinking it

6d40511

klimeryk reviewed Feb 7, 2016
View reviewed changes

nylen added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 10, 2016

klimeryk pushed a commit that referenced this pull request Feb 11, 2016

Merge pull request #2124 from Automattic/add/windows-support

f87045a

Build: fixes for making Calypso run on Windows under MSYS2

klimeryk merged commit f87045a into master Feb 11, 2016

klimeryk deleted the add/windows-support branch February 11, 2016 01:44

scruffian removed the [Status] Ready to Merge label Feb 11, 2016

nylen mentioned this pull request Feb 16, 2016

install instructions for PC .exe #2029

Closed

rralian mentioned this pull request Feb 25, 2016

Framework: use npm scripts instead of make for build system #3520

Closed

This was referenced Mar 10, 2016

Build: Remove quasi-dependency on semver package #3964

Closed

Build: Remove quasi-dependency on semver package (try 2) #4002

Merged

nylen added a commit to Automattic/wp-api-console that referenced this pull request Nov 12, 2016

Minor readme updates

0267848

A couple of typo fixes. Also switch to `https` for git URL - previously: - Automattic/wp-calypso#2124 (comment) - Automattic/wp-calypso#2278

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: fixes for making Calypso run on Windows under MSYS2 #2124

Build: fixes for making Calypso run on Windows under MSYS2 #2124

klimeryk commented Jan 5, 2016

klimeryk Jan 5, 2016

blowery Jan 5, 2016

klimeryk Jan 5, 2016

nylen Feb 4, 2016

Pomax Feb 25, 2016

aduth Feb 25, 2016

Pomax Feb 25, 2016

nylen Feb 26, 2016

Pomax Feb 26, 2016

nylen Feb 26, 2016

pmualaba commented Jan 6, 2016

dmsnell commented Jan 7, 2016

alexdresko commented Jan 7, 2016

Viper007Bond commented Jan 7, 2016

klimeryk commented Jan 8, 2016

localpcguy commented Jan 18, 2016

klimeryk commented Jan 18, 2016

liquidboy commented Jan 20, 2016

klimeryk commented Jan 20, 2016

liquidboy commented Jan 20, 2016

klimeryk commented Jan 20, 2016

ebinnion Feb 5, 2016

ebinnion Feb 5, 2016

klimeryk Feb 6, 2016

nylen commented Feb 6, 2016

ebinnion commented Feb 7, 2016

klimeryk commented Feb 7, 2016

klimeryk Feb 7, 2016

klimeryk Feb 7, 2016

nylen commented Feb 10, 2016

klimeryk commented Feb 11, 2016

Build: fixes for making Calypso run on Windows under MSYS2 #2124

Build: fixes for making Calypso run on Windows under MSYS2 #2124

Conversation

klimeryk commented Jan 5, 2016

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

pmualaba commented Jan 6, 2016

dmsnell commented Jan 7, 2016

alexdresko commented Jan 7, 2016

Viper007Bond commented Jan 7, 2016

klimeryk commented Jan 8, 2016

localpcguy commented Jan 18, 2016

klimeryk commented Jan 18, 2016

liquidboy commented Jan 20, 2016

klimeryk commented Jan 20, 2016

liquidboy commented Jan 20, 2016

klimeryk commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Feb 6, 2016

ebinnion commented Feb 7, 2016

klimeryk commented Feb 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Feb 10, 2016

klimeryk commented Feb 11, 2016