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

Typecheck Hack projects on push #5

Open
wants to merge 1 commit into
base: hhvm
Choose a base branch
from
Open

Conversation

jwatzman
Copy link

And reject the push if typechecking fails. In the future we may want to also
look for a Hack project entirely missing an .hhconfig file, but this will do
for now.

And reject the push if typechecking fails. In the future we may want to also
look for a Hack project entirely missing an .hhconfig file, but this will do
for now.
@pvh
Copy link

pvh commented Apr 25, 2014

Looks good!

dzuelke added a commit that referenced this pull request May 8, 2014
@RdeWilde
Copy link

Very cool! Could it problems at deploy-time causing a timeout? If so it would be usefull to have an option to disable it (or is there already).

@jwatzman
Copy link
Author

Very cool! Could it problems at deploy-time causing a timeout? If so it would be usefull to have an option to disable it (or is there already).

To be clear, you're concerned that the typecheck will take too long, and hit some sort of timeout during deploy? How long is that timeout? Unless it is very short, I'm not too concerned. The checker is pretty efficient -- we can check Facebook's code (>10M LOC) from scratch in about 40 seconds on a 32-core machine. While Heroku is probably running the deploy on a machine an order of magnitude less beefy, projects being deployed are probably many orders of magnitude smaller. Since the time scales roughly linearly with both LOC and CPUs, I'd expect a check time of only maybe 5-10 seconds on the largest projects, with the vast majority being significantly faster than that.

I'd also prefer not to have an option to disable this, if we can get away with it. It's very important that Hack errors really are treated as real errors, the sort that you absolutely would not want to deploy with. They are definite inconsistencies in the code. Here at Facebook, a type error in trunk is considered to be "trunk is broken" -- and the only reason this is a post-commit check instead of a real commit hook is technicalities having to do with how exactly our commit process works. A type error in the production branch will block a push; this pretty regularly catches bugs from non-textual merge conflicts and the like.

Do those numbers make sense to you? It's easily possible I've mis-estimated something or am otherwise wrong here :)

PS: sorry for the delayed response, I somehow missed the github notification for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants