Openenclave: Speed up pre-commit hook

[andschwa]

We have this pre-commit hook that CI runs. Its name and function implies it was meant as a Git pre-commit hook, but it's rather slow for this (and the readme doesn't actually document that it can/should(?) be installed as a pre-commit hook).

I'll see if I can speed the scripts up.

/cc @anitagov

[0dragosh]

@andschwa I've been looking into this, and it seems that the pre-commit hook script itself only takes about 20 seconds to run, the rest of the ~70 seconds are all Jenkins pipeline operations behind the scenes (since it is still a declarative pipeline).

Parallelizing the scripts that are run in the pre-commit hook script only yielded about 1 second of gain in performance. I've also had a look at the tools that are used in each individual script and scoured their manuals for optimization flags, but I couldn't find anything relevant. I think the pre-commit hook script is as optimized as it can be, under the current requirements.

PR #1085 changes the whole pipeline from declarative to scripted, so I'm hoping we might see some performance gains there. I'll come back to this after that PR is merged to see whether or not it's faster.

[andschwa]

@dragoshsch 20 seconds is wayyy too much for anyone to run for each and every commit 😄

Currently this script _only runs in CI because it takes 20 seconds. It _should_ be run automatically for each commit as a pre-commit hook by developers on their machine, but they can't because it's so crazy slow.

[andschwa]

Make it run as a pre-commit hook to only run against the changed files (available in environment variables from the hook).

Probably still want the option of running the scripts against the whole repo, but even then, they should utilize git ls-files instead of a bunch of find magic.

Please don't let me forgot to post my WIP branch. It wasn't working perfectly, but it'd refactored a fair bit of these scripts;

[andschwa]

@dragoshsch Here is the the WIP branch. It wasn't quite working; as I dug into the implementation I found a lot of oddities.

Also, I think for each of the three scripts, we need to retain the option of running against all files in the repo. So like, if a list of files is specified as input, just check those (and then we'd use this in the pre-commit hook), and if no files are specified, use git ls-files (and we'd use this in the CI where the speed doesn't matter).

Once we're happy with the script, I can update the docs to explain how developers can/should enable the hook.

@johnkord Seem reasonable?

[johnkord]

@andschwa Sounds great to me. I like the idea of running these checks against all files in the CI, but running only on the different files locally for the pre-commit hook.

[0dragosh]

@andschwa I've managed to get the code from your branch to work in my commit here, here's the diff comparing with your code. As you can see, there are only a few minor (but very tricky and time consuming) differences.

Unfortunately, this version of the check-license script is about 2 minutes slower than the original code 😞 .

@andschwa @johnkord Should I just proceed with implementing the options to run the precommit scripts just against the files given as arguments (and leave it to run against all the files if no file arguments are provided -- for the CI)?

[andschwa]

Unfortunately, this version of the check-license script is about 2 minutes slower than the original code 😞 .

Oh no!

@andschwa @johnkord Should I just proceed with implementing the options to run the precommit scripts just against the files given as arguments (and leave it to run against all the files if no file arguments are provided -- for the CI)?

Yes please 😄

[0dragosh]

@andschwa I created PR #1197 for this. Please have a look when you can, and test it out and let me know how it would work out for you in your day-to-day development workflow.

cc @johnkord
Thanks!

[ionutbalutoiu]

The proposed pull request was merged as part of https://github.com/Microsoft/openenclave/pull/1270. Closing this issue associated to it.