Make the build / run / test process Windows-compatible by adding NPM scripts for all the tasks. #13536

DanReyLop · 2017-05-01T21:38:54Z

This PR is a split of #12583, a bit smaller so it's easier to review and less risky.

This PR keeps the Makefile and all its tasks intact. So now, both systems
(NPM and make) should work, but NPM is used everywhere (CircleCI, Dockerfile, css-hot-loader).

To keep the PR reasonably small, I've ommited any documentation changes.

cc/ @blowery @gziolo @gwwar @stephanethomas

matticbot · 2017-05-01T21:38:55Z

Test live: https://calypso.live/?branch=try/build-with-npm-scripts

gziolo · 2017-05-02T06:40:13Z

From CircleCi:

It's present in other branches, too ... :)

gziolo · 2017-05-02T06:50:51Z

server/bundler/bin/bundler.js

 	outputOptions;

+if ( 'development' === config( 'env' ) ) {


It looks like bundleEnv wasn't used at all before. Why do we want to exit early for development env?

In the Makefile, the bundler is only run when env != development, here: https://github.com/Automattic/wp-calypso/blob/master/Makefile#L188

That (conditionals) is hard to replicate in NPM scripts, so I opted for always running the bundler (here) and exit early if env == development.

Cool 👍 Now it makes perfect sense :)

gziolo · 2017-05-02T06:54:38Z

server/devdocs/bin/generate-components-usage-stats.js

@@ -8,9 +8,10 @@

 var async = require( 'async' ),
 	camelCase = require( 'lodash/camelCase' ),
-	config = require( 'config' ),
+	config = require( '../../config' ),


Does it break otherwise? This config thing is really confusing :(

No, it doesn't break. I probably made this change before I figured out how NODE_PATH works. I've just reverted it to remove some noise.

gziolo · 2017-05-02T07:07:31Z

I testes the following command on macOS Sierra:

npm run analyze-bundles
npm run clean
npm run distclean
npm start
npm test

Everything works as expected.

I'm wondering if pre commands always work properly. In the case of npm run analyze-bundles it does't execute prebuild (npm run -s install-if-needed). See:

Grzegorzs-MacBook-Pro:wp-calypso gziolo$ npm run analyze-bundles

wp-calypso@0.17.0 preanalyze-bundles /Users/gziolo/PhpstormProjects/wp-calypso
cross-env WEBPACK_OUTPUT_JSON=1 CALYPSO_ENV=production npm run -s build

Rendering Complete, saving .css file...
Wrote CSS to /Users/gziolo/PhpstormProjects/wp-calypso/public/directly.css
Rendering Complete, saving .css file...
Wrote CSS to /Users/gziolo/PhpstormProjects/wp-calypso/public/editor.css
Rendering Complete, saving .css file...
Wrote CSS to /Users/gziolo/PhpstormProjects/wp-calypso/public/style.css
Building: proptypes-index.json
Rendering Complete, saving .css file...
Wrote Source Map to /Users/gziolo/PhpstormProjects/wp-calypso/public/style-debug.css.map
Wrote CSS to /Users/gziolo/PhpstormProjects/wp-calypso/public/style-debug.css
Saving: /Users/gziolo/PhpstormProjects/wp-calypso/public/style-rtl.css
Time: 21s 838.901ms

DanReyLop · 2017-05-02T07:10:55Z

I'm wondering if pre commands always work properly. In the case of npm run analyze-bundles it does't execute prebuild (npm run -s install-if-needed).

That's expected. It is executing prebuild, it just isn't showing in the console. I've added -s (--silent) to all the npm run commands so running a build isn't too noisy. So, if you run npm run command, in the console you'll only see lines for precommand, command and postcommand, not for the dependant subcommands.

gziolo · 2017-05-02T07:12:52Z

It needs more testing, but otherwise it looks awesome 👍

stephanethomas

I had very minor comments, but otherwise the code looks good.

stephanethomas · 2017-05-02T16:02:57Z

bin/trim-shrinkwrap.js

+ */
+
+const fs = require( 'fs' );
+const shrinkwrap = require( __dirname + '/../npm-shrinkwrap.json' );


Do we really have to use __dirname here? What about the following?

const shrinkwrap = require( '../npm-shrinkwrap.json' );

You're right, it's not needed. Fixed.

stephanethomas · 2017-05-02T16:07:44Z

package.json

@@ -32,6 +32,7 @@
    "bounding-client-rect": "1.0.5",
    "browser-filesaver": "1.1.0",
    "chalk": "1.0.0",
+    "check-node-version": "2.0.1",


This might be a typo: the latest version is 2.1.0.

Not a typo, 2.0.1 was the latest version until 2 days ago :)

Bumped to the latest.

stephanethomas · 2017-05-02T16:09:38Z

package.json

@@ -61,11 +63,12 @@
    "flux": "2.1.1",
    "fuse.js": "2.6.1",
    "get-video-id": "2.1.4",
+    "globby": "3.0.1",


What about using the latest version, which is 6.1.0?

This one may have been a typo... 3.0.1 is quite old.

Bumped to the latest.

stephanethomas · 2017-05-02T16:59:56Z

I tried on Ubuntu and I could run Calypso without any apparent issue. I tried the following commands:

npm install
npm start
npm run eslint-branch
npm run lint
npm run update-deps

There is this strange error about Yarn when I run npm start though:

> wp-calypso@0.17.0 prestart /var/sources-others/calypso
> npm run -s install-if-needed && ( check-node-version --package || exit 0 )

node: 6.10.2
npm: 3.10.10
/bin/sh: 1: yarn: not found

DanReyLop · 2017-05-02T17:20:22Z

There is this strange error about Yarn when I run npm start though:

Did you try it after my latest change? It was a bug that was fixed in the latest version of check-node-version: parshap/check-node-version#11

It's not really an error because we don't have a yarn version requirement listed in package.json, but that output can be confusing.

stephanethomas · 2017-05-02T17:36:24Z

Did you try it after my latest change? It was a bug that was fixed in the latest version of check-node-version

No I didn't test that change. Glad to hear it was fixed!

matticbot · 2017-05-05T02:37:46Z

@DanReyLop This PR needs a rebase

matticbot · 2017-05-08T15:01:39Z

@DanReyLop This PR needs a rebase

DanReyLop · 2017-05-08T17:59:13Z

So... is this ready to ship? 2 people tested it in OS X and Ubuntu with no apparent issues, and CI is passing (using the new build commands).

matticbot · 2017-05-09T18:09:32Z

@DanReyLop This PR needs a rebase

matticbot · 2017-05-19T08:22:08Z

@DanReyLop This PR needs a rebase

DanReyLop · 2017-05-19T20:21:09Z

If there is no objection, I plan to merge this PR next Wednesday at noon UTC.

Since Dockerfile uses the make build command, the risk to the actual production build is minimal. CircleCI has been running using the new npm scripts, without any issue. Plus, 2 people have already tested it.

gziolo · 2017-05-20T10:38:39Z

I'll give it another try on Monday or Tuesday :)

gziolo · 2017-05-22T12:26:37Z

gziolo · 2017-05-22T12:27:46Z

When loading http://calypso.localhost:3000/read/a8c I see the following error on JS console:

It probably needs another rebase with master.

gziolo · 2017-05-22T12:33:51Z

After executing npm run update-deps I have modified npm-shrinkwrap.json file. It would be a good idea to update regenerate it again in this branch just before merging it with master.

Technically npm run lint:js works, but it errors. At the same time Circle CI build passes, so I'm assuming there are differences in configuration.

gziolo · 2017-05-22T12:37:02Z

circle.yml

@@ -5,9 +5,9 @@ test:
  pre:
    - ? |
          # make the build-server and i18n string data in parallel
-          if [[ "$CIRCLE_NODE_INDEX" == 0 ]]; then NODE_ENV=test make build-server; fi
+          if [[ "$CIRCLE_NODE_INDEX" == 0 ]]; then NODE_ENV=test npm run build-server; fi


Can we revert changes made to Circle CI config file and put them in the follow up PR?

We could, but... why? CircleCI is passing using the new build system, which gives some reassurance that it works.

gziolo

I think we should double check why linter fails locally and remove changed applied to Circle CI config file. We should also make sure Calypso works properly. It is probably some unlucky coincidence that Reader errors.

Once this is done I'm happy to approve this patch.

DanReyLop · 2017-05-22T14:06:54Z

Technically npm run lint:js works, but it errors. At the same time Circle CI build passes, so I'm assuming there are differences in configuration.

npm run lint:js runs it on the whole codebase (which still has a ton of lint errors), while CircleCI only lints the files that were modified in the branch. A more practical command to run locally is npm run eslint-branch.

npm run lint:css isn't run by CircleCI at all, it's giving hundreds of errors since forever (in master too).

After executing npm run update-deps I have modified npm-shrinkwrap.json file. It would be a good idea to update regenerate it again in this branch just before merging it with master.

Done :)

When loading http://calypso.localhost:3000/read/a8c I see the following error on JS console:

I've merged with master again, that solved it for me.

# Conflicts: # npm-shrinkwrap.json # package.json

gwwar · 2017-05-24T17:23:08Z

package.json

+    "test-test": "cross-env NODE_ENV=test NODE_PATH=test:client TEST_ROOT=test node test/runner.js",
+    "test-test:watch": "nodemon -e js,jsx --exec npm run -s test-test",
+    "translate": "i18n-calypso --format pot --output-file ./calypso-strings.pot -e date \"**/*.js\" \"**/*.jsx\" \"!build/**\" \"!node_modules/**\" \"!public/**\" \"!client/extensions/**\"",
+    "update-deps": "npm run -s rm -- node_modules && npm run -s rm -- npm-shrinkwrap.json && npm install && npm install && npm shrinkwrap --dev"


@DanReyLop Sorry, I missed this earlier, but do you mind adding a --no-optional flag to each of the install commands in a follow up PR? eg npm install --no-optional

Currently this will add fsevents to the shinkwrap that will throw errors in the docker image.

See also #5386

Is that still a problem? npm 3.10.9 and lower had that issue, but it was fixed in 3.10.10 (which is what we're using). See: https://github.com/npm/npm/releases/tag/v3.10.10

Nice, it no longer tries to build fsevents from scratch. Ignore my above comment then. 😄

I also tested to see if we still need the double install, and it looks like that has finally been fixed too, so we can cut this down to one npm install

Nice! I'll gladly remove yet another hack on my next PR :)
I never had the "double install" problem, so I wasn't sure how did it manifest itself.

After we updated to npm 3, it used to show up very consistently after you removed node_modules + the shrinkwrap. Basically you'd npm install and then a second npm install would install more stuff!

Looks like it was closed out a while ago, see: npm/npm#10727

@gwwar Just to be on the safe side, I won't remove the second npm install. According to this PR: npm/npm#12811 (comment) the fix was included in npm 4.0.5, so we'll better wait till October :)

Sounds good to me 👍

astralbodies · 2017-07-05T13:21:45Z

Was the desktop app build tested with this PR? I'm having problems building on CircleCI but not on my macOS machine.

DanReyLop · 2017-07-05T13:57:45Z

@astralbodies I tried funning make build-desktop, but not building the full wp-desktop. What error are you getting?

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label May 1, 2017

DanReyLop mentioned this pull request May 1, 2017

Remove our dependency with "make", make it easier to develop on Windows #12583

Closed

6 tasks

DanReyLop added Build Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 1, 2017

gziolo requested review from blowery, gwwar and stephanethomas May 2, 2017 06:42

gziolo reviewed May 2, 2017

View reviewed changes

DanReyLop force-pushed the try/build-with-npm-scripts branch from 7cdd335 to 17c750b Compare May 2, 2017 07:04

stephanethomas reviewed May 2, 2017

View reviewed changes

matticbot added the [Status] Needs Rebase label May 5, 2017

DanReyLop removed the [Status] Needs Rebase label May 8, 2017

matticbot added the [Status] Needs Rebase label May 8, 2017

DanReyLop removed the [Status] Needs Rebase label May 8, 2017

matticbot added the [Status] Needs Rebase label May 9, 2017

DanReyLop removed the [Status] Needs Rebase label May 9, 2017

DanReyLop removed the [Status] Needs Rebase label May 18, 2017

matticbot added the [Status] Needs Rebase label May 19, 2017

Merge branch 'master' into try/build-with-npm-scripts

dfd19e4

DanReyLop removed the [Status] Needs Rebase label May 19, 2017

Merge branch 'master' into try/build-with-npm-scripts

120890d

gziolo reviewed May 22, 2017

View reviewed changes

gziolo added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 22, 2017

gziolo requested changes May 22, 2017

View reviewed changes

Daniel Rey Lopez added 2 commits May 22, 2017 14:34

Merge branch 'master' into try/build-with-npm-scripts

3c33c81

Updated shrinkwrap.

983d62d

DanReyLop added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels May 24, 2017

Merge branch 'master' into try/build-with-npm-scripts

4131d65

# Conflicts: # npm-shrinkwrap.json # package.json

DanReyLop merged commit 45dcdae into master May 24, 2017

DanReyLop deleted the try/build-with-npm-scripts branch May 24, 2017 16:22

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 24, 2017

gwwar reviewed May 24, 2017

View reviewed changes

DanReyLop mentioned this pull request May 29, 2017

Changed Dockerfile to use "npm run build" instead of "make build" #14546

Merged

This was referenced Aug 9, 2017

Framework: Remove Makefile #17033

Merged

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

Closed

Make the build / run / test process Windows-compatible by adding NPM scripts for all the tasks. #13536

Make the build / run / test process Windows-compatible by adding NPM scripts for all the tasks. #13536

Conversation

DanReyLop commented May 1, 2017

matticbot commented May 1, 2017

gziolo commented May 2, 2017 • edited Loading

gziolo May 2, 2017 • edited Loading

Choose a reason for hiding this comment

DanReyLop May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanReyLop May 2, 2017 • edited Loading

Choose a reason for hiding this comment

gziolo commented May 2, 2017 • edited Loading

DanReyLop commented May 2, 2017

gziolo commented May 2, 2017

stephanethomas left a 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

stephanethomas commented May 2, 2017

DanReyLop commented May 2, 2017

stephanethomas commented May 2, 2017

matticbot commented May 5, 2017

matticbot commented May 8, 2017

DanReyLop commented May 8, 2017

matticbot commented May 9, 2017

matticbot commented May 19, 2017

DanReyLop commented May 19, 2017

gziolo commented May 20, 2017

gziolo commented May 22, 2017 • edited Loading

gziolo commented May 22, 2017

gziolo commented May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

DanReyLop commented May 22, 2017

Choose a reason for hiding this comment

DanReyLop May 24, 2017 • edited Loading

Choose a reason for hiding this comment

gwwar May 24, 2017 • edited Loading

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

astralbodies commented Jul 5, 2017

DanReyLop commented Jul 5, 2017

gziolo commented May 2, 2017 •

edited

Loading

gziolo May 2, 2017 •

edited

Loading

DanReyLop May 2, 2017 •

edited

Loading

DanReyLop May 2, 2017 •

edited

Loading

gziolo commented May 2, 2017 •

edited

Loading

gziolo commented May 22, 2017 •

edited

Loading

gziolo commented May 22, 2017 •

edited

Loading

DanReyLop May 24, 2017 •

edited

Loading

gwwar May 24, 2017 •

edited

Loading