Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.42k stars 1.99k forks source link

Build: improve build startup speed and maintainability #23252

Closed gwwar closed 5 years ago

gwwar commented 6 years ago

We now have 67 npm scripts! Folks have been finding this:

Let's look into some options in how we may improve this! We should aim for something that is quick, doesn't require a lot of thinking to maintain, and if we choose a 3rd party tool, is one that is relatively popular and will have long term support.

Going back to a Makefile is also an option, but adds extra system requirements for Windows development

See also, for additional context: p4TIVU-8QD-p2 https://href.li/?https://github.com/Automattic/wp-calypso/issues/3520

cc @Automattic/team-calypso

samouri commented 6 years ago

Before we jump to a solution like Make I'd like to better understand the problem.

slow startup

Slow startup of what exactly? All of our scripts have different "startup" time usually due to "pre" steps. They also all have different runtimes. Are there particular scripts that feel too slow

  • hard to maintain
  • slow and difficult to parallelize

Do we have any examples of modifications that were hard to implement due to the number of npm scripts? Or any examples of operations we want to parallelize that we are having difficulty doing due to the constraints of npm?


I don't mean to derail or slow down progress but I do want to make sure we clearly understand what problems we are trying to solve

gwwar commented 6 years ago

cc @nylen @dmsnell @withinboredom @DanReyLop to help clarify points for @samouri

Note that I was trying to paraphrase from the p2 post on pain points. Clarifying questions are great @samouri ! Let's update the summary afterwards (anyone is welcome to take a shot).

DanReyLop commented 6 years ago

Disclaimer: While I acknowledge all the points below, I don't think anyone is a deal-breaker or impossible to solve while keeping using multi-platform JS tools.

All of our scripts have different "startup" time usually due to "pre" steps.

The "pre" steps aren't really the problem. They're just there to have some logical separation. pretask, task and posttask is basically equivalent to pretask && task && posttask. You have the ability to run npm run pretask explicitly so it's a bit better for debugging the build steps, but other than that it's irrelevant.

Slow startup of what exactly?

Every time you run npm run something a new npm process is spawn, files are parsed, etc. The actual command that's on the something task is run in the system shell (bash/cmd.exe), so that's yet another layer of indirection. Combine it with the sheer amount of npm run commands that are run for the build, and it adds up.

Are there particular scripts that feel too slow

Startup is slower because everything is rebuilt from scratch. The way Makefile works, it doesn't execute a task if its target was generated by the latest versions of its dependencies (it checks the files' mtime I think, the same way the install-if-deps-outdated.js script does). So if you run make run, stop it and then run it again without changing any SCSS file, then the build-css steps wouldn't run at all. Same with other steps of the build. npm doesn't have that kind of stuff built-in.

hard to maintain

There are several sub-points to this. There are a lot of NPM scripts, so touching them seems daunting. It doesn't help that they're sorted alphabetically instead of by functionality. In an npm script command, you're limited to a single line of bash. If you want anything more complex, you need to offloaded to a bin/run-whatever.js script. Which I don't think it's a bad solution, but I digress.

Windows compatibility introduces some annoyances too. Altough you can get quite far writing commands that work in bash and cmd.exe, differences in environments are why we have cross-env and bin/rm.js. That makes the npm commands harder to read an modify, and they add yet another layer of sub-process creation. Sometimes you end up having to create a JS script for something you could slap together in a bash one-liner.

dmsnell commented 6 years ago

All of our scripts have different "startup" time usually due to "pre" steps Do we have any examples of modifications that were hard to implement due to the number of npm scripts?

Our "pre" steps are just that - pre steps - they add needles work before actual computation. One thing that is hard to do is something like this trivial line in make

test-client: $(JS_FILES)
    jest -c test/config/jest.client.js

the npm scripts have no concept of prerequisites or dependencies and so they just rebuild everything every time unless some tool keeps another isolated cache of work it's already done.

similarly the version check only needs to run if node or npm change…

version-check: $(NPM) $(NODE)
    …

or shave off mucho measurable milliseconds by only running prettier on files that actually changed and are part of the commit

lint: $(filter %.js %.jsx %.scss,$(shell git diff --name-only --diff-filter=ACM)
    prettier --write $?

or run a utility that only needs to make sure a single npm package is installed to run instead of all of them…

lint-css: $(FILES_CSS) | node_modules/stylelint
    $(NPM_BIN)stylelint --syntax sass $(filter %.scss,$?)

or share code and processing among the different stages. make's variables are easy to reuse and we can easily adjust between lazy and eager evaluation of those variables as we need.

and we can easily overwrite the number of parallel tasks from the command-line for a number of tasks, -j1 or -j4 or -j32 etc… or let makehandle it with -l to set a load-average to maintain

the point isn't so much about making more things happen at the same time - it's about making fewer things happen. our npm scripts are Javascript and make isn't; there's no doubt about that, but they waste so much and cause so much needless delay.

all you have to do is look at the code that #23225 removes to see some of the crazy extent we went to in order to abandon make, which only needed a little love in order to meet our needs - we did not abandon make because it was't working well - we abandoned it because someone found it personally easier to rebuild everything in npm scripts than make the changes we needed in order to get the Makefile working well in windows. it appears that those necessary changes were quite minimal anyway.

23225 takes inspiration from our original Makefile and is there to resolve the windows issues and it also reworks some things to be more efficient where easy. So far the tasks I have moved over are operating at just about as fast as they possibly can: that it, they do as little duplicated work as is necessary.

jsnajdr commented 6 years ago

@dmsnell Could tools like Gulp or Grunt help achieve the goals as well, instead of make? I'm not pushing any of them, just trying to inform myself.

A few things about your examples that I'd like to understand better:

test-client: $(JS_FILES)
    jest -c test/config/jest.client.js

Does this run jest on all the JS_FILES one by one? Or only on the changed ones? Or something else?

version-check: $(NPM) $(NODE)

Does this run the version check only if the dependent executables change? There must be a version-check file generated by this rule to make it work, or doesn't it? If not, where does make get the timestamps to compare against?

gwwar commented 6 years ago

I don't think anyone is a deal-breaker or impossible to solve while keeping using multi-platform JS tools.

If the original summary was unclear, apologies! Using multi-platform JS tools is totally an option and my preference if we can get a good solution going. I just wanted to list current pain points, and see if folks had ideas on ways forward. If we do move toward something else we should vet a few options and be comfortable with the decision.

DanReyLop commented 6 years ago

we did not abandon make because it was't working well - we abandoned it because someone found it personally easier to rebuild everything in npm scripts than make the changes we needed in order to get the Makefile working well in windows. it appears that those necessary changes were quite minimal anyway. #23225 takes inspiration from our original Makefile and is there to resolve the windows issues and it also reworks some things to be more efficient where easy.

@dmsnell Since it's clear who are you talking about here, I'll bite. What you are doing now is not making the Makefile compatible with Windows. Your Makefile, at least in its current state, shares the same caveats as the old one. On Windows, you'll need to install a "UNIX-like" toolchain (MSys2, Cygwin) so you get not only make, but also other UNIX tools like find. Also you'll need to install programs in non-standard locations (at least git, because something in make doesn't handle spaces in paths well and the default path for installing programs is C:\Program Files\). I tried to follow those instructions when I started working here and I found it extremely challenging as an A11N, I can't imagine many OSS contributors would jump through those hoops in order to contribute.

Or, you could install WSL, which is basically a full Ubuntu userspace embedded in Windows, so it behaves almost as a Linux VM. In both scenarios, the old Makefile would work. Detecting the OS and changing the path separator to \ doesn't magically make a UNIX tool multi-platform. Which is fine, we decided that we don't want to spend time supporting Windows, but let's not delude ourselves thinking that bringing back a Makefile wouldn't raise the barrier of entry for Windows folks.

dmsnell commented 6 years ago

Could tools like Gulp or Grunt help achieve the goals as well, instead of make?

yes they could @jsnajdr though the major drawback to those is that they tend to have quite verbose syntax and the ecosystem is always changing. again, I think at some point we're running to those things because they are JavaScript and not necessarily because they serve our needs well. we already depend on git which isn't JavaScript and having make installed is one of those things that pretty much every dev box needs anyway.

a cursory examination among our existing dependencies shows a number of JS libraries using make:

it's not the end of the world to use it is all I'm saying and it brings major advantages, one of which is that it's universally standard and hasn't made a major breaking change in decades. any actual build tool is going to be better than what we have and it's also going to bring some level of complexity and a new language. even the JS build tools are entirely separate languages; they just don't depend on non-JS code (probably).

heck, we could compile make via emscripten for all I care and use it just as well (but at that point why bother?)

Does this run jest on all the JS_FILES one by one? Or only on the changed ones? Or something else?

it runs jest according to how we currently do - on all files. the reason is to preserve our tests' ability to catch unintended breakages across long coupling. if we wanted to only run it on changed files we would simply supply a $? at the end of the jest line and that would overwrite the value in the jest config - that $? means "the set of prerequisites which have changed more recently than the current target" so in most PRs it will be just a small number of files.

Does this run the version check only if the dependent executables change? There must be a version-check file generated by this rule to make it work, or doesn't it? If not, where does make get the timestamps to compare against?

Sorry - the actual version check was omitted for to save space. I have a reasonably verbose recipe that started out much simpler but this one colors the major version in the output.

It compares the modification time of the prerequisites so in this case that would be the npm and node binaries. If they haven't changed then unless we have intentionally forged the filesystem we shouldn't need to recalculate the versions - that's the heart of make's language. Turns out that file modification stamps can get us really far with little complexity.

What you are doing now is not making the Makefile compatible with Windows.

@DanReyLop sorry if I misunderstood you when we were testing the other day. per Slack I thought we concluded that WSL was a reasonable dependency for Windows and that it provided everything we needed for compatibility, just the same way macOS users also have to install something to get git (could be git directly or could be Xcode or brew etc…). I don't know how big WSL is but it seems like a kind of hammer that takes care of all these issues as you mention, thus meaning that it's not a blocker. At last, the way I understood what people were saying is that WSL is essentially a single dependency in Windows that when installed removes almost all of the compatibility issues.

Detecting the OS and changing the path separator to \ doesn't magically make a UNIX tool multi-platform

Also this was never the intention. Those things are necessary which is why they are there, but the goal is to be reasonably windows compatible, even before we talked about WSL. A big part of portability is doing more of the work inside the make language. things like $(wildcard …) and $(filter …) and such are portable and are fast because they don't require booting up another node or shell process.


install-if-deps-outdated.js is a good example of the extra work we're doing and shows how a proper system can make our maintenance easier - it's also portable

node_modules: npm-shrinkwrap.json
    npm install
#!/usr/bin/env node

/**
 * Performs an `npm install`. Since that's a costly operation,
 * it will only perform it if needed, that is, if the packages
 * installed at `node_modules` aren't in sync over what
 * `npm-shrinkwrap.json` has. For that, modification times of both
 * files will be compared. If the shrinkwrap is newer, it means that
 * the packages at node_modules may be outdated. That will happen,
 * for example, when switching branches.
 */

const fs = require( 'fs' );
const spawnSync = require( 'child_process' ).spawnSync;

const needsInstall = () => {
    try {
        const shrinkwrapTime = fs.statSync( 'npm-shrinkwrap.json' ).mtime;
        const nodeModulesTime = fs.statSync( 'node_modules' ).mtime;
        return shrinkwrapTime - nodeModulesTime > 1000; // In Windows, directory mtime has less precision than file mtime
    } catch ( e ) {
        return true;
    }
};

if ( needsInstall() ) {
    const installResult = spawnSync( 'npm', [ 'install' ], {
        shell: true,
        stdio: 'inherit',
    } ).status;
    if ( installResult ) {
        process.exit( installResult );
    }
    fs.utimesSync( 'node_modules', new Date(), new Date() );
}
DanReyLop commented 6 years ago

per Slack I thought we concluded that WSL was a reasonable dependency for Windows and that it provided everything we needed for compatibility

Well, it's more reasonable than installing MSys2, configure it right, running pacman -whatever install make, re-installing git because the default install path has spaces, reboot (maybe?) and pray you followed all the instructions right. WSL solves the Windows compatibility problem in the same way that a Ubuntu VM would solve the Windows compatibility problem. Which is to say that it's ok, at least it's one extra thing you need to install, but it's not really fair to claim that the build system is cross-platform.

I don't know how big WSL is but it seems like a kind of hammer that takes care of all these issues as you mention, thus meaning that it's not a blocker.

I haven't tried it, but it looks big, like hundreds of MBs big. Which I guess it's like requiring XCode if you're looking for an analogy.

At last, the way I understood what people were saying is that WSL is essentially a single dependency in Windows that when installed removes almost all of the compatibility issues.

It solves all compatibility problems. With WSL, you're building in something like a full Ubuntu machine.

Also this was never the intention. Those things are necessary which is why they are there, but the goal is to be reasonably windows compatible, even before we talked about WSL. A big part of portability is doing more of the work inside the make language. things like $(wildcard …) and $(filter …) and such are portable and are fast because they don't require booting up another node or shell process.

My point is that a Makefile can't be portable if make itself isn't portable. The path separator thing isn't needed because make will always be run in UNIX or in a fake simulated UNIX-like environment, which will have paths with / separator. Unless this miracolously works (last updated 12 years ago), then the "only" problems would be that the Makefile uses bash commands like find.

PS: I've just fallen in line and got a Mac so this doesn't affect me personally anymore :)

withinboredom commented 6 years ago

I would not focus on the portability of make, we can have some good WSL docs on how to get it setup (maybe even a bash script that installs all the deps right out of the box for Ubuntu/RH WSL installs -- bonus points because it will help out people running those linux distros too).

In regards to the original points, I opened https://github.com/Automattic/wp-calypso/pull/23323 -- it's nowhere near a complete rendition. I need to go to bed, so I'm not 100% sure it actually works; I did it merely for illustration purposes.

stale[bot] commented 5 years ago

This issue has been marked as stale and will be closed in seven days. This happened because:

You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation.

dmsnell commented 5 years ago

We know things are slow; we are occupied putting out fires; we know why we got to this point and hopefully some day we'll make things better. Issue closed.