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

Make lint fast again #938

Closed
wants to merge 4 commits into from
Closed

Make lint fast again #938

wants to merge 4 commits into from

Conversation

MikeBishop
Copy link
Contributor

@MikeBishop MikeBishop commented Nov 16, 2017

@ianswett and I were remarking that makes and commits have gotten very slow lately. Some observation of my machine shows that I'm spending a lot of that time checking for trailing whitespace and long lines.

This updates the make lint recipe to cache the hashes per branch of files that have successfully been linted and not re-lint them unless they've been modified (or you're on a new branch -- sorry). On my machine, lint goes from 12 seconds to 0.75 seconds.

@MikeBishop MikeBishop added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Nov 16, 2017
@MikeBishop
Copy link
Contributor Author

MikeBishop commented Nov 17, 2017

You can make it do the same for new branches too, but it requires a git post-checkout hook, which is a local configuration (unless we get the i-d-template to bring the hook along like it does the pre-commit hook). Here's the hook script:

#!/bin/bash

new_lintcache=".lintcache/`git rev-parse --abbrev-ref HEAD`";

if [ ! -d "$new_lintcache" ]; then
    for branch in `git branch --points-at $2`; do
        old_lintcache=".lintcache/$branch";
        if [ -d "$old_lintcache" ]; then
            cp -r $old_lintcache $new_lintcache;
            break;
        fi
    done
fi

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that there is a simpler design.

Have lint depend on .lintcache/filename and generate that file as a result of linting filename. Let make do all the heavy lifting.

The downside of that approach is that make will stop after the first error. We can fix that on CI at least by using make -k on an explicit linting step.

@janaiyengar
Copy link
Contributor

(sorry, nothing useful to offer at the end of a long IETF week, but I really want to rename this "Make lint fast again").

@MikeBishop
Copy link
Contributor Author

You're right, that's a better design. Also, I can fold the branch migration into that -- check at lint time whether I can steal a lint file from another branch rather than actually running the lint commands.

I'll rearchitect that later. We should also perhaps update the Circle config to cache .lintcache as well. Does that mean I need to make the directory controllable by an env variable?

@MikeBishop MikeBishop changed the title Make lint faster Make lint fast again Nov 17, 2017
@MikeBishop
Copy link
Contributor Author

There you go, @janaiyengar. ;-)

@martinthomson
Copy link
Member

Caching .lintcache won't work because the code is checked out from scratch every time, so modification times are updated. Paying the price on CI isn't that bad for the moment.

@MikeBishop
Copy link
Contributor Author

I think it should, actually, because the content of the .lint_cache file is the hash of the Markdown file. If the file hasn't changed in the latest commit, all you pay is the hashing cost to double-check that.

@MikeBishop MikeBishop dismissed martinthomson’s stale review November 17, 2017 07:51

Make now does as you suggested.

@martinthomson
Copy link
Member

I don't think that optimizing for re-runs in CI is worthwhile. And maybe you misunderstood my suggestion. This is much more complicated than I think we need. I'll show you what I mean with another PR.

@martinthomson martinthomson mentioned this pull request Nov 27, 2017
@MikeBishop
Copy link
Contributor Author

Closing in favor of #1032.

@MikeBishop MikeBishop closed this Jan 4, 2018
@MikeBishop MikeBishop deleted the lint_cache branch January 21, 2018 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants