Framework: updated makefile to work with single build.js #275

samouri · 2017-03-06T13:47:41Z

Along with the wp-calypso PR here (which has already landed), this PR seeks make wp-desktop compatible with the latest changes to wp-calypso (single build for non-development envs).

Testing

Checkout ~~try/framework/ssr-config-desktop-compat~~ master Calypso branch.
Run make test to verify it works properly.
Run make run and check if it builds and works properly.

gziolo · 2017-03-07T06:02:10Z

desktop/app-handlers/crash-reporting/index.js

@@ -11,10 +11,10 @@ const debug = require( 'debug' )( 'desktop:crash-reporting' );
 /**
 * Internal dependencies
 */
-const Config = require( 'lib/config' );
+const Config = require( '../../lib/config' );


nice workaround :)

Agreed :)
I don't see any problem with a relative path for desktop's internals!
An alternative that I've been playing with is just aliasing desktop so that this becomes:

const Config = require( 'desktop/lib/config' );

Though I don't think this actually adds much value over a relative path, beyond just allowing further nested files use the same reference and avoid the '../../../../xx type paths.

gziolo · 2017-03-07T06:04:25Z

desktop/window-handlers/external-links/index.js

@@ -9,7 +9,7 @@ const debug = require( 'debug' )( 'desktop:external-links' );
 /**
 * Internal dependencies
 */
-const Config = require( 'lib/config' );
+const Config = require( '../../lib/config' );


There are some many places that need to use this strange construct. Maybe we should move this folder elsewhere or rename it to something else? :D

there would definitely be less friction if we change lib/config in wp-calypso to be /lib/create-config. I'm hoping we can come up with a cleaner solution like somehow having wp-desktop higher up in the NODE_PATH

spen · 2017-03-09T22:11:00Z

desktop/app-handlers/crash-reporting/index.js

@@ -11,10 +11,10 @@ const debug = require( 'debug' )( 'desktop:crash-reporting' );
 /**
 * Internal dependencies
 */
-const Config = require( 'lib/config' );
+const Config = require( '../../lib/config' );


Agreed :)
I don't see any problem with a relative path for desktop's internals!
An alternative that I've been playing with is just aliasing desktop so that this becomes:

const Config = require( 'desktop/lib/config' );

Though I don't think this actually adds much value over a relative path, beyond just allowing further nested files use the same reference and avoid the '../../../../xx type paths.

spen · 2017-03-09T22:14:40Z

desktop/app-handlers/crash-reporting/index.js


 module.exports = function() {
-	if ( Config.crash_reporter.electron ) {
+	if ( Config.crash_reporter && Config.crash_reporter.electron ) {


I recall seeing errors from this line in my recent attempts to merge desktop > calypso so this is nice to see.
One concern that I have though: is there a chance that this'll give us false positives?
My guess is that it won't and that this change is of benefit, but I just wanted to raise the question anyway :)

Also, I'll keep these in mind as once the projects are merged we'll have access to lodash's util methods here.
I think get(Config, 'crash_reporter.electron') might work nicely at that point.

I recall seeing errors from this line in my recent attempts to merge desktop > calypso so this is nice to see.

We were probably getting the error for the same reason: the wrong config was being imported. I'm hoping in the end this change isn't necessary

spen · 2017-03-09T22:18:52Z

Makefile

 	@rm -f $(THIS_DIR)/calypso/public/devmodules.*

 build-if-not-exists:
-	@if [ -f $(CALYPSO_JS_STD) ]; then true; else make build; fi
+	@if [ -f $(CALYPSO_JS) ]; then true; else make build; fi


Makes sense!

gziolo · 2017-03-10T07:49:50Z

const Config = require( 'desktop/lib/config' );

I don't think it is a good idea to reference only one file located insidedesktop folder with alias and omit that alias for the rest. There might be some issue with incorrect NODE_PATH setup. Maybe it would be enough to change the order of folders that are scanned when trying to load a file?

We have /desktop/index.js set as an entry point:
https://github.com/Automattic/wp-desktop/blob/master/webpack.config.js#L12

and calypso/client as NODE_PATH:
https://github.com/Automattic/wp-desktop/blob/master/Makefile#L94

We execute make package from the root file. It means we look for lib/config in the following places:

/lib/config/ - file not found
/calypso/client/lib/config - it finds file here

If we would modify NODE_PATH value with NODE_PATH=desktop:calypso/client it should solve the issue we see here. I didn't test it, but I think we should give it a try :)

samouri · 2017-03-10T22:20:55Z

@gziolo, if we can get the NODE_PATH working properly that would definitely be the cleanest option (although the alias is pretty explicit which is nice). I'll see if I can get it working via NODE_PATH

samouri · 2017-03-13T22:27:02Z

getting an error i've never seen now where req.context === undefined
EDIT: I believe this is due to a recent commit to server/pages/render.js within wp-calypso.

gziolo · 2017-03-14T12:42:54Z

Makefile

@@ -91,17 +96,15 @@ endif
 package: build-if-changed
 	@echo "Bundling app and server"
 	@rm -rf $(BUILD_DIR)/public_desktop $(BUILD_DIR)/calypso
-	@NODE_PATH=calypso/client $(WEBPACK_BIN) --config $(THIS_DIR)/webpack.config.js
+	@NODE_PATH=calypso/server$(SEPARATOR)calypso/client $(WEBPACK_BIN) --config $(THIS_DIR)/webpack.config.js


We probably should move NODE_PATH handling to Webpack config. @gwwar @blowery @aduth could you help with this?

i think this is used here because the webpack config requires config which is found via the NODE_PATH.

gziolo · 2017-03-14T12:53:27Z

I added a few changes which resolve all issues we have. I don't think it is perfect solution, but it should allow us to move forward. I had to update NODE_PATH specified in Makefile to make sure server config is loaded in calypso/client/sections.js file, the same way as it is done in Calypso.

@mtias @blowery: do you know why calypso/client/sections.js is located into client folder, but always updated as part of bundler which is executed from server? Can we move it to calypso/server to keep it closer to the actual code using it? It might be tricky because of how Webpack rewrites this file.

blowery · 2017-03-14T13:28:13Z

do you know why calypso/client/sections.js is located into client folder, but always updated as part of bundler which is executed from server?

I think it's because it's require'd into the client. It holds all of the section paths and their associated page path hooks. Webpack generates it (statically for productiony builds, dynamically for development), but the client uses it.

Yeah, the fact that it gets "overwritten" is a bit odd...

mtias · 2017-03-14T13:58:10Z

We have been wanting to rename client to app for a bit. There's a lot of pieces that run both on the client and the server there.

mkaz · 2017-03-14T16:49:59Z

Makefile

+SEPARATOR := :
+endif
+
+


Minor but the SEPARATOR variable might be confused with path separator / or \ especially with how it's used below - may I suggest ENV_PATH_SEP or something similar.

Potentially stupid question ahead:
What is the benefit of using $(SEPARATOR) in the first place? Is this so that we can use a different separator per system? ( I think I read that windows uses ; for instance)?
Or is there something beyond this?

Is this so that we can use a different separator per system?

nailed it. see https://github.com/Automattic/wp-desktop/pull/275/files#diff-b67911656ef5d18c4ae36cb6741b7965R1

I copied it over from Calypso. I remember that there were some issues with make commands on Windows. In the long run we should resolve those kind of issues by using npm scripts 😄

samouri · 2017-03-15T01:06:14Z

Makefile

@@ -91,17 +96,15 @@ endif
 package: build-if-changed
 	@echo "Bundling app and server"
 	@rm -rf $(BUILD_DIR)/public_desktop $(BUILD_DIR)/calypso
-	@NODE_PATH=calypso/client $(WEBPACK_BIN) --config $(THIS_DIR)/webpack.config.js
+	@NODE_PATH=calypso/server$(ENV_PATH_SEP)calypso/client $(WEBPACK_BIN) --config $(THIS_DIR)/webpack.config.js


solved 99% of the problem with the line right here. good work @gziolo 👍

samouri · 2017-03-15T04:02:37Z

Seems to be working for me except for make test. I can't tell if that usually works though..

gziolo · 2017-03-15T07:18:02Z

If we would modify NODE_PATH value with NODE_PATH=desktop:calypso/client it should solve the issue we see here. I didn't test it, but I think we should give it a try :)

I have to disagree with myself here :) I spent couple of hours debugging this issue. It turned out that when you execute Webpack magic it loads code from desktop subfolder, but also uses code located incalypso/server which itself depends on calypso/client. We were using before lib/config from desktop but alo from client depending on the context. It needs to remain this way that's why it was easier to rename lib/config in client folder to lib/create-config to avoid naming collisions. Other change I introduced, NODE_PATH change in Makefile, ensures we always use server/config when executing Calypso code in its context. Desktop has also its own config which gets generated during packaging and can be found in desktop/config.json file.

astralbodies · 2017-03-17T11:47:00Z

@gziolo Any chance you had time to look back at this one?

gziolo · 2017-03-20T11:14:35Z

@astralbodies looking into it now.

…o config

gziolo · 2017-03-20T12:55:35Z

Makefile

 	@echo "Copying Calypso client and public files"
 	@sed -e 's/build\///' $(THIS_DIR)/package.json >$(BUILD_DIR)/package.json
 	@mkdir $(BUILD_DIR)/calypso $(BUILD_DIR)/calypso/config $(BUILD_DIR)/calypso/server
 	@cp -R $(THIS_DIR)/public_desktop $(BUILD_DIR)
 	@cp -R $(CALYPSO_DIR)/public $(BUILD_DIR)/calypso/public
 	@cp -R $(CALYPSO_DIR)/server/pages $(BUILD_DIR)/calypso/server/pages
 	@if [ -f $(CALYPSO_DIR)/config/secrets.json ]; then cp $(CALYPSO_DIR)/config/secrets.json $(BUILD_DIR)/calypso/config/secrets.json; else cp $(CALYPSO_DIR)/config/empty-secrets.json $(BUILD_DIR)/calypso/config/secrets.json; fi;
+	@cp $(CALYPSO_DIR)/config/_shared.json $(BUILD_DIR)/calypso/config/


It looks like make test needs all config files to build config file properly. Those config files aren't needed for make run, but might be needed for packaging. Let's leave them as they were.

Without this config files located in build folder make test isn't able to load properly configuration and were trying to load web version of Calypso. This is why it couldn't load properly in test mode :)

gziolo · 2017-03-20T12:57:41Z

@astralbodies I fixed make test. I don't know how to test make osx and family. It complains that I have wrong secret-clientid. It expects 43452 as app id, which is probably what we use on production packages. I have my own auth id app configured.

I did a diff between the last desktop release and the one I built - a big standout to me is that the config folder isn't in the bundle - secrets.json and desktop.json are missing.

You were completely right. The only good part is that I find out that we don't copy over _shared.json config file ;)

gziolo · 2017-03-20T13:03:05Z

I have just noticed that we set node 6.1.0 on Circle CI. Updated to 6.10.0 to make CI more reliable 😄

It looks like it is required to make test work.

astralbodies

OSX build runs for me!

gziolo · 2017-03-21T08:06:59Z

Should I merge it as it is or does it need more testing?

astralbodies · 2017-03-21T12:41:07Z

@gziolo I vote for you merging it in now, and then I can do a full beta release process from master.

astralbodies · 2017-03-21T12:53:18Z

I did just note one problem with make test-osx even though make osx seems to work properly:

Packaging app for platform darwin x64 using electron v1.3.5

Optimizing the Mac OS X build at /Users/aaron/src/wp-desktop/release/WordPress.com-darwin-x64/WordPress.com.app
 - Renaming helper plist files
 - Removing default app
 - Signing app
fs.js:640
  return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode);
                 ^

Error: ENOENT: no such file or directory, open './release/WordPress.com-darwin-x64/WordPress.com.app/Contents/Resources/app.asar'
    at Error (native)
    at Object.fs.openSync (fs.js:640:18)
    at Object.module.exports.readArchiveHeaderSync (/Users/aaron/src/wp-desktop/node_modules/asar/lib/disk.js:79:13)
    at Object.module.exports.readFilesystemSync (/Users/aaron/src/wp-desktop/node_modules/asar/lib/disk.js:105:21)
    at Object.module.exports.extractAll (/Users/aaron/src/wp-desktop/node_modules/asar/lib/asar.js:186:23)
    at Command.<anonymous> (/Users/aaron/src/wp-desktop/node_modules/asar/bin/asar:59:15)
    at Command.listener (/Users/aaron/src/wp-desktop/node_modules/asar/node_modules/commander/index.js:301:8)
    at emitTwo (events.js:106:13)
    at Command.emit (events.js:191:7)
    at Command.parseArgs (/Users/aaron/src/wp-desktop/node_modules/asar/node_modules/commander/index.js:615:12)
make: *** [test-osx] Error 1
aaron@astralimac [07:52:04] [~/src/wp-desktop] [try/ssr-config-compat]
-> %

astralbodies · 2017-03-21T13:36:14Z

@gziolo I fixed the paths in the test target but the osx.js test is requiring a directory that doesn't exist and I'm not sure what to update it with. Any idea @mkaz?

[warn] kq_init: detected broken kqueue; not using.: Undefined error: 0
[warn] kq_init: detected broken kqueue; not using.: Undefined error: 0
[warn] kq_init: detected broken kqueue; not using.: Undefined error: 0
{ Error: Cannot find module '../../release/WordPress.com-darwin-x64-unpacked/desktop/app'
    at Module._resolveFilename (module.js:440:15)
    at Function.Module._resolveFilename (/Users/aaron/src/wp-desktop/node_modules/electron/dist/Electron.app/Contents/Resources/electron.asar/common/reset-search-paths.js:35:12)
    at Function.Module._load (module.js:388:25)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/aaron/src/wp-desktop/resource/test/osx.js:12:14)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:458:32)
    at tryModuleLoad (module.js:417:12)
    at Function.Module._load (module.js:409:3)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at /Users/aaron/src/wp-desktop/node_modules/mocha/lib/mocha.js:220:27
    at Array.forEach (native)
    at Mocha.loadFiles (/Users/aaron/src/wp-desktop/node_modules/mocha/lib/mocha.js:217:14)
    at Mocha.run (/Users/aaron/src/wp-desktop/node_modules/mocha/lib/mocha.js:469:10)
    at Object.run (/Users/aaron/src/wp-desktop/release/WordPress.com-darwin-x64-unpacked/node_modules/electron-mocha/mocha.js:65:28)
    at App.app.on (/Users/aaron/src/wp-desktop/release/WordPress.com-darwin-x64-unpacked/node_modules/electron-mocha/index.js:36:11)
    at emitOne (events.js:101:20)
    at App.emit (events.js:188:7) code: 'MODULE_NOT_FOUND' }
Error: Cannot find module '../../release/WordPress.com-darwin-x64-unpacked/desktop/app'
    at Module._resolveFilename (module.js:440:15)
    at Function.Module._resolveFilename (/Users/aaron/src/wp-desktop/node_modules/electron/dist/Electron.app/Contents/Resources/electron.asar/common/reset-search-paths.js:35:12)
    at Function.Module._load (module.js:388:25)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/aaron/src/wp-desktop/resource/test/osx.js:12:14)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:458:32)
    at tryModuleLoad (module.js:417:12)
    at Function.Module._load (module.js:409:3)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at /Users/aaron/src/wp-desktop/node_modules/mocha/lib/mocha.js:220:27
    at Array.forEach (native)
    at Mocha.loadFiles (/Users/aaron/src/wp-desktop/node_modules/mocha/lib/mocha.js:217:14)
    at Mocha.run (/Users/aaron/src/wp-desktop/node_modules/mocha/lib/mocha.js:469:10)
    at Object.run (/Users/aaron/src/wp-desktop/release/WordPress.com-darwin-x64-unpacked/node_modules/electron-mocha/mocha.js:65:28)
    at App.app.on (/Users/aaron/src/wp-desktop/release/WordPress.com-darwin-x64-unpacked/node_modules/electron-mocha/index.js:36:11)
    at emitOne (events.js:101:20)
    at App.emit (events.js:188:7)
make: *** [test-osx] Error 1

astralbodies · 2017-03-22T16:43:05Z

I am taking @johngodley's suggestion and not letting the make test-osx failures block this PR. I am going to merge this into master so I can move forward with the desktop build release. I'll open an issue to address the test target in the Makefile.

gziolo · 2017-03-22T18:41:26Z

@astralbodies thanks for help with testing :)

samouri added the [Status] In Progress label Mar 6, 2017

samouri self-assigned this Mar 6, 2017

samouri force-pushed the try/ssr-config-compat branch from c780b88 to 5cd8b53 Compare March 6, 2017 13:48

samouri mentioned this pull request Mar 6, 2017

SSR The config module Automattic/wp-calypso#11352

Merged

4 tasks

gziolo reviewed Mar 7, 2017

View reviewed changes

spen reviewed Mar 9, 2017

View reviewed changes

spen mentioned this pull request Mar 9, 2017

Webpack: Add alias for "desktop" #276

Closed

samouri mentioned this pull request Mar 14, 2017

Attempt at making ssr-config compatible with wp-desktop Automattic/wp-calypso#11785

Merged

gziolo reviewed Mar 14, 2017

View reviewed changes

gziolo force-pushed the try/ssr-config-compat branch from 75918a5 to 44cafa2 Compare March 14, 2017 12:48

gziolo requested review from gwwar, mkaz and johngodley March 14, 2017 13:04

mkaz reviewed Mar 14, 2017

View reviewed changes

samouri commented Mar 15, 2017

View reviewed changes

gziolo added [Status] Needs Review [Pri] Blocking [Type] Bug and removed [Status] In Progress labels Mar 15, 2017

gziolo changed the title ~~updated makefile to work with single build.js~~ Framework: updated makefile to work with single build.js Mar 15, 2017

gziolo requested a review from astralbodies March 15, 2017 07:23

samouri and others added 7 commits March 20, 2017 13:50

updated makefile to work with single build.js

82ee264

make all configs relative. this is a bad hacky solution

c3de9d2

Build: Revert relative path to lib/config in desktop folder

d5a3834

Build: Add changes to Makefile to make build work with updated Calyps…

edf7881

…o config

removed unecessary changes

eb7de32

SEPARATOR-->ENV_PATH_SEP @mkaz

5961545

Point at current master.

e38b934

gziolo force-pushed the try/ssr-config-compat branch 2 times, most recently from d8adc5b to e5b0a58 Compare March 20, 2017 12:53

gziolo reviewed Mar 20, 2017

View reviewed changes

gziolo added 2 commits March 20, 2017 14:11

Build: Revert change which no longer adds Calypso config files to build

6e1bb7d

It looks like it is required to make test work.

Framework: Update node version on Circle CI

a85c2a5

gziolo force-pushed the try/ssr-config-compat branch from 023679d to a85c2a5 Compare March 20, 2017 13:11

astralbodies approved these changes Mar 20, 2017

View reviewed changes

Fixes paths on two parts of the test-osx target. Tests still fail.

2dd8169

seear mentioned this pull request Mar 21, 2017

Themes: Enable showcase 'magic search' in desktop app Automattic/wp-calypso#12376

Merged

astralbodies merged commit dfd1f8b into master Mar 22, 2017

astralbodies deleted the try/ssr-config-compat branch March 22, 2017 16:43

matticbot removed the [Status] Needs Review label Mar 22, 2017

spen mentioned this pull request Mar 27, 2017

Desktop: Merge wp-desktop in to Calypso - First Steps Automattic/wp-calypso#12430

Closed

29 tasks

Framework: updated makefile to work with single build.js #275

Framework: updated makefile to work with single build.js #275

Conversation

samouri commented Mar 6, 2017 • edited by gziolo Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Mar 7, 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

Choose a reason for hiding this comment

gziolo commented Mar 10, 2017 • edited Loading

samouri commented Mar 10, 2017 • edited Loading

samouri commented Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Mar 14, 2017

blowery commented Mar 14, 2017 • edited Loading

mtias commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

samouri Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

samouri commented Mar 15, 2017

gziolo commented Mar 15, 2017

astralbodies commented Mar 17, 2017

gziolo commented Mar 20, 2017

Choose a reason for hiding this comment

gziolo commented Mar 20, 2017 • edited Loading

gziolo commented Mar 20, 2017

astralbodies left a comment

Choose a reason for hiding this comment

gziolo commented Mar 21, 2017

astralbodies commented Mar 21, 2017

astralbodies commented Mar 21, 2017

astralbodies commented Mar 21, 2017 • edited Loading

astralbodies commented Mar 22, 2017

gziolo commented Mar 22, 2017

samouri commented Mar 6, 2017 •

edited by gziolo

Loading

samouri Mar 7, 2017 •

edited

Loading

gziolo commented Mar 10, 2017 •

edited

Loading

samouri commented Mar 10, 2017 •

edited

Loading

samouri commented Mar 13, 2017 •

edited

Loading

blowery commented Mar 14, 2017 •

edited

Loading

gziolo Mar 15, 2017 •

edited

Loading

samouri Mar 15, 2017 •

edited

Loading

gziolo commented Mar 20, 2017 •

edited

Loading

astralbodies commented Mar 21, 2017 •

edited

Loading