Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Allow incremental rebuilding of previews #1643

Merged
merged 5 commits into from Jun 25, 2017

Conversation

Projects
None yet
4 participants
Contributor

harding commented Jun 23, 2017

Currently, when previewing a change to the site with make preview, one needs to rebuild every page for each test change. For a rebuild with all optional plugins disabled, this takes 27 seconds on my laptop; for a full rebuild with all plugins enabled, this takes 293 seconds.

This PR changes make preview to use what Jekyll calls "incremental rebuilds" where only pages detected as changing are rebuilt. The time to rebuild varies by what you change---for example, a layout change that affects all pages will take as long as a complete rebuild---but for a single-page change, I find this takes about 7 seconds on my laptop with all optional plugins disabled, although 240 seconds with all plugins enabled.

I think those times could be improved further (on BitcoinCore.org where they don't have homebrew plugins, incremental rebuilds with all plugins enabled take less than a second for a single page change), but I think going from waiting 27 seconds to 7 seconds to preview a change is a major improvement for people who work on editing the site locally.


Technical notes: all Bitcoin.org plugins were written for Jekyll 0.5 but we currently use Jekyll 3.0, so three of the plugins had to be changed in order to enable incremental rebuilding.

  1. The events and contributors plugins were loaded non-idempotently. Specifically, on the initial load, they each monkey-patch a Jekyll function; on reloads during incremental rebuilds, they attempt to monkey patch the function they already money patched, causing the build process to lock. The first commit fixes this by suppressing the additional monkey patching.

  2. The less CSS plugin used to turn nested stylesheets into valid CSS stylesheets deleted its own output on successive runs. This was fine when we always did full rebuilds, but it meant that after an incremental rebuild we had no stylesheets. Instead of fixing the plugin, which was needed when we used Jekyll 0.5, I changed to using SCSS/SASS nested stylesheet support that is included by default in Jekyll 3.0.

    The second commit implements both the previous LESS and new SCSS side-by-side so that the results can be diffed. Here's the diff; I only see whitespace changes. The third commit removes less, and the fourth commit compacts the SCSS output to save bandwidth.

The final commit turns on incremental rebuilding when make preview is run.

Except for the CSS now being more compressed than before, the content of the site should be unchanged. Preview here: http://dg0.dtrt.org/

harding added some commits Jun 7, 2017

Build: allow repeated rebuilding in Jekyll Preview mode
The events and contributor plugins both monkey patched the `site`
object.  This worked fine when they were loaded once per site build, but
with Jekyll 3.0 automatic site rebuilding in preview and watch modes,
this applied the monkey patch recursively, causing the program to halt.

With this commit, the monkey patching should only occur once per run.
Layout: render CSS with both "less" and "scss"
This commit renders the same CSS files using both the "less" and "scss"
CSS generation engines so that the results may be compared.  This is a
comparison script:

    old_main_css=$( grep -rl normalize.css _site/*.css )
    new_main_css=_site/css/main.css
    old_rtl_css=$( grep 'Language specific styles that override default' -rl _site/*.css )
    new_rtl_css=_site/css/rtl.css

    (
      diff -ub $old_main_css $new_main_css
      diff -ub $old_rtl_css $new_rtl_css
    )
Contributor

crwatkins commented Jun 23, 2017

Concept ACK.

@wbnns wbnns self-assigned this Jun 24, 2017

Contributor

wbnns commented Jun 24, 2017

@harding This is great, thanks Dave.

Unless others object, this will be merged on Sunday, June 25th.

Contributor

laanwj commented Jun 24, 2017

This is awesome. I suppose combined with Travis caching this could also reduce the turn-around time for the checks.

Contributor

harding commented Jun 24, 2017

@laanwj If we enabled caching, this PR would only reduce Travis time by 10% according to some quick profiling I just did. That's not enough IMO to justify a different testing process than the live site build process.

If I improve the build further, then using caching and incremental rebuilds for both Travis and the live site build would seem warranted. The main goal for this PR is selfish: I've been doing a lot of editing on the dev guide, and I got tired of waiting 27 seconds to see my changes. Just 7 seconds is much better.

In the meantime, I'll see what I can do for the Bitcoin Core binaries URL check that you use. That currently runs about 300 seconds into the full build process, but if I move that check into the releases plugin itself, it'll run about 15 seconds into the full build process---giving you a faster fail in case of problems. (I'll do that in a separate PR.)

@wbnns wbnns merged commit 25f6b2a into bitcoin-dot-org:master Jun 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Contributor

laanwj commented Jun 25, 2017

@harding OK, fair enough. Yes my main issue would be that the Travis check turnaround time when adding a new release takes long. This isn't a problem when it's only to be done once, but as we regularly have to fix small HTML/Markdown issues in the release notes this can take a lot of time.

@Mirobit Mirobit referenced this pull request Jun 29, 2017

Merged

Make preview charset fix #1651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment