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

Reduce the number of terminals required to build riot-web to 1 #7355

Merged
merged 7 commits into from Sep 25, 2018

Conversation

@turt2live
Copy link
Member

turt2live commented Sep 17, 2018

This is a work in progress.

todo:

  • Fix race condition where the react-sdk initial build fails sometimes
  • Fix race condition where the riot-web initial build fails sometimes
  • Fix race condition where riot-web builds 5+ times during the initial build

A step towards a real solution for #7305

This approach makes use of npm link to remove the use of symlinks in the build process. The build process has also been altered to invoke the build process of each underlying SDK (react, js). This means that one can now npm link and npm start and have a working environment.

At the same time, parallelshell was dropped due to lack of maintenance from the maintainer.

Requires:

turt2live added 3 commits Sep 17, 2018
A step towards a real solution for #7305

This approach makes use of `npm link` to remove the use of symlinks in the build process. The build process has also been altered to invoke the build process of each underlying SDK (react, js). This means that one can now `npm link` and `npm start` and have a working environment. 

At the same time, parallelshell was dropped due to lack of maintenance from the maintainer.
If we don't block on SDK builds, then the riot-web build fails due to half-built dependencies. This needs to be done at two levels: the js-sdk because it is used by both the react-sdk and riot-web, and at the react-sdk because riot-web needs it. This means our build process is synchronous for js -> react -> riot, at least for the initial build. 

This does increase the startup time, particularly because the file watch timer is at 5 seconds. The timer is used to detect a storm of file changes in the underlying SDKs and give the build process some room to compile larger files if needed. 

The file watcher is accompanied by a "canary signal file" to prevent the build-blocking script from unblocking too early. Both the js and react SDKs build when `npm install` is run, so we ensure that we only listen for the `npm start` build for each SDK.

This is all done at the riot level instead of at the individual SDK levels (where we could use a canary file to signal up the stack) because:
* babel (used by the js-sdk) doesn't really provide an "end up build" signal
* webpack is a bit of a nightmare to get it to behave at times
* this blocking approach is really only applicable to riot-web, although may be useful to some other projects.

Hopefully that all makes sense.
@turt2live turt2live changed the title [WIP] Reduce the number of terminals required to build riot-web to 1 Reduce the number of terminals required to build riot-web to 1 Sep 24, 2018
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Sep 24, 2018

Race conditions addressed 🎉

Edit: There's some very lengthy prose in 2b037ee to describe wtf is going on

@turt2live turt2live requested a review from vector-im/riot-web Sep 24, 2018
clearTimeout(timerId);
}
//console.log(`Waiting ${WAIT_TIME}ms for another file update...`);
timerId = setTimeout(() => {

This comment has been minimized.

Copy link
@ara4n

ara4n Sep 24, 2018

Member

Hm, i'm surprised that we pause 5s at this point, and i'm failing to see why...

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Sep 24, 2018

this generally lgtm, although i don't understand why we need the 5s timeout given we're doing the swanky (dying) canary thing.

also, the comment in 2b037ee would really benefit from going in a comment block somewhere, perhaps at the top of block-on-sdk-build.sh?

exciting!

Copy link
Member

ara4n left a comment

5s?

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Sep 24, 2018

I'll reword and put the comment block in somewhere.

We do a 5s timeout for file changes because the canary only indicates when we should actually start watching for file changes. The high level process looks like npm install (causes a build) -> canary -> start watching for changes -> npm start for an SDK. The 5s timeout is there to give the npm start (webpack/babel) enough time to process larger files and ensure the process is actually finished. In my testing, this could go as low as 1 or 1.5s, however that caused some instability. 5s was arbitrarily chosen as "safe", and means the initial build is only delayed by ~10s.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Sep 24, 2018

why can't we fire the canary once npm start has stopped thrashing?

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Sep 24, 2018

Because babel (used by the js-sdk) doesn't provide a good place to do this, and webpack (used by the react-sdk) doesn't seem to want to behave and do that. block-on-sdk-build.js effectively does the job though by waiting for the thrashing to stop before letting the process continue, and works with any build system.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Sep 25, 2018

right, i see. i guess one alternative could be to chuck a dummy file called zz9.js into js-sdk/src and rely on the fact that babel traverses the FS in alphabetical order, and use js-sdk/lib/zz9.js as the canary to tell once babel has finished its thing? this looks like it could also work in react-sdk, which seems to be using babel rather than webpack?

My concern about the 5s is mainly the risk of slower/overloaded comps or CI systems taking longer and a general aversion to 'sticking a sleep in it', as well as wanting first experiences to be as snappy as possible.

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Sep 25, 2018

CI should be unaffected as this doesn't touch that path, however 5s is fairly arbitrary indeed. Relying on alphabetical ordering of babel feels slightly more magical than a sleep to me, and doesn't really give the code base a good image (imo) when there's a magic file included in the source tree.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Sep 25, 2018

we could put something really useful in the file to make it less magical ;) but yup, agreed that relying on filesystem ordering being alphabetic is very dubious. and having looked at babel's source, agreed there's no point where it gives you a hook to tell you when it's finished the initial build :|

Another idea: how about running babel twice in the js-sdk & react-sdk; once with without -w, then touch/kill a canary, and then again with -w --skip-initial-build? Then you don't need to worry about looking at churn at all, and can just keep an eye on the canary?

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Sep 25, 2018

That sounds like a safer option (and also uses a lot less magic watching). I'll give that a shot tomorrow and see how it goes, expecting it to be snappier.

With the react-sdk and js-sdk having their `npm start`s split out (as per matrix-org/matrix-react-sdk#2175 and matrix-org/matrix-js-sdk#742) we can trigger an initial build ourselves and start the watcher afterwards. This canary approach has a very slight speed increase over serially running all the commands as the watcher can be started as early as possible.

This all can be improved and potentially eliminated with a bit more planning, however: #7386
@turt2live turt2live requested a review from ara4n Sep 25, 2018
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Sep 25, 2018

lgtm modulo the thoughts you already filed over at #7386, assuming it works :)

@ara4n
ara4n approved these changes Sep 25, 2018
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Sep 25, 2018

lgtmer. (i have slight concerns about what npm link is buying us here; but i guess if the user wishes to manually symlink rather than using npm link they can)

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Sep 25, 2018

merging so i can use it :P

@ara4n ara4n merged commit 841c8fd into develop Sep 25, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.