Rewrite makefile to be reliable #4034
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
force-pushed
the
rewrite_makefile
branch
from
33397d9 to
a30fe6a
Compare
github-actions
bot
force-pushed
the
rewrite_makefile
branch
from
bc79ebc to
4ad2544
Compare
github-actions
bot
force-pushed
the
rewrite_makefile
branch
from
87e6d1c to
950347a
Compare
vytautas-karpavicius
approved these changes
Mar 8, 2021
Makefile
Outdated
| # - Targets should never, EVER be *actual source files*. | ||
| # Always use book-keeping files in $(BUILD). | ||
| # Otherwise e.g. changing git branches could confuse Make about what it needs to do. | ||
| # - Simiarly, prerequisites should be those book-keeping files, |
There was a problem hiding this comment.
typo: Similarly
Makefile
Outdated
| # | ||
| # this is not fatal, we can just run 2x. | ||
| # to be fancier though, we can detect when *both* are run, and re-touch the book-keeping files to prevent the second run. | ||
| # this STRICTLY REQUIRES that `copyright` and `fmt` are mututally stable, and that copyright runs before fmt. |
There was a problem hiding this comment.
typo: mututally
There was a problem hiding this comment.
bleh. I should hunt down a spellchecker extension. kinda surprised vscode doesn't come with one tbh, or use the OS's.
Strict and correct and parallelizable. I think it even reads better. Next up: fix tests, and document.
I did a cleanup pass, tested a bunch, seems reliable and ready to share. Just for funsies, on clean builds (though the go module cache is populated): ``` > time make bins ... make bins 104.18s user 38.68s system 310% cpu 45.941 total > time make bins -j8 ... make bins -j8 108.43s user 53.00s system 717% cpu 22.489 total ``` Twice as fast when parallelized!
github-actions
bot
force-pushed
the
rewrite_makefile
branch
from
f3a6401 to
612a074
Compare
vytautas-karpavicius
pushed a commit
that referenced
this pull request
Mar 15, 2021
Phase 2 of makefile cleanups! As a tl;dr, what this PR does is: - makes the high-level build steps explicit so they're easier to follow (right near the top) - switches to Make-only book-keeping files, so steps are reliably recalculated in all situations - moves tool-bins to a .bin folder so they are not deleted by default, and versions them more strictly (you can now switch between versions for "free", great for e.g. trying new protocs) - adds a _reliable_ way to avoid codegen in buildkite / wherever it's needed or desired As a bonus, this should now be safe to always run in parallel, which runs about 2x as fast for me: ``` > time make bins ... make bins 104.18s user 38.68s system 310% cpu 45.941 total > time make bins -j8 ... make bins -j8 108.43s user 53.00s system 717% cpu 22.489 total ``` I'll change Buildkite to do this in a bit, when it has been battle-tested a bit more. Next (and final) phase is to make test-running a bit more normal, but that needs a bit more work. --- As a brief summary about why book-keeping files are important, be aware of these two details: 1. Make _only_ looks at file modification times to determine what to do. If A depends on B and B is newer than A (or A does not exist), it'll re-build A. 2. Git (and other tools) _do not_ record modification times, to work better with Make. This is why `touch go.mod` doesn't lead to anything to commit - git doesn't record _when_ you modified it, only what the modifications were. Because of this, on e.g. a brand new `git clone`, _all_ files are "the same" age... with some randomness about what ones are newer than others, within e.g. milliseconds. So as far as Make is concerned, a bunch of _random_ files have been modified. If you had makefile rules like `output.go: input.go \n do codegen`, some of those output files may be newer than their inputs, so they may or may not be re-made. That's pretty clearly wrong. The same kind of thing happens when you change branches - depending on what changed between your before/after commits, those files are now "new". The way out of this madness is to rely on files that e.g. Git does _not_ modify: these book-keeping files. - When cloning (or clean), they do not exist, so they must all be re-made. - When changing branches, any changed files are _always_ newer than these book-keeping files, so they will _always_ be re-made (when appropriate). So that's what this PR does.
yux0
pushed a commit
to yux0/cadence
that referenced
this pull request
May 4, 2021
Phase 2 of makefile cleanups! As a tl;dr, what this PR does is: - makes the high-level build steps explicit so they're easier to follow (right near the top) - switches to Make-only book-keeping files, so steps are reliably recalculated in all situations - moves tool-bins to a .bin folder so they are not deleted by default, and versions them more strictly (you can now switch between versions for "free", great for e.g. trying new protocs) - adds a _reliable_ way to avoid codegen in buildkite / wherever it's needed or desired As a bonus, this should now be safe to always run in parallel, which runs about 2x as fast for me: ``` > time make bins ... make bins 104.18s user 38.68s system 310% cpu 45.941 total > time make bins -j8 ... make bins -j8 108.43s user 53.00s system 717% cpu 22.489 total ``` I'll change Buildkite to do this in a bit, when it has been battle-tested a bit more. Next (and final) phase is to make test-running a bit more normal, but that needs a bit more work. --- As a brief summary about why book-keeping files are important, be aware of these two details: 1. Make _only_ looks at file modification times to determine what to do. If A depends on B and B is newer than A (or A does not exist), it'll re-build A. 2. Git (and other tools) _do not_ record modification times, to work better with Make. This is why `touch go.mod` doesn't lead to anything to commit - git doesn't record _when_ you modified it, only what the modifications were. Because of this, on e.g. a brand new `git clone`, _all_ files are "the same" age... with some randomness about what ones are newer than others, within e.g. milliseconds. So as far as Make is concerned, a bunch of _random_ files have been modified. If you had makefile rules like `output.go: input.go \n do codegen`, some of those output files may be newer than their inputs, so they may or may not be re-made. That's pretty clearly wrong. The same kind of thing happens when you change branches - depending on what changed between your before/after commits, those files are now "new". The way out of this madness is to rely on files that e.g. Git does _not_ modify: these book-keeping files. - When cloning (or clean), they do not exist, so they must all be re-made. - When changing branches, any changed files are _always_ newer than these book-keeping files, so they will _always_ be re-made (when appropriate). So that's what this PR does.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Phase 2 of makefile cleanups!
I've added some simple docs to the makefile, hopefully they're easy to follow.
For higher-level docs and general Make education (e.g. why, exactly, I recommend so strongly about never "making" source files)... I need to find a location, and it'll take a little longer to write. I'm kinda fond of a
makefile.mdorarchitecture.mdbut I'm not yet sure where to put it.But as a tl;dr, what this PR does is:
As a bonus, this should now be safe to always run in parallel, which runs about 2x as fast for me:
I'll change Buildkite to do this in a bit, when it has been battle-tested a bit more.
Next (and final) phase is to make test-running a bit more normal, but that needs a bit more work.
As a brief summary about why book-keeping files are important, be aware of these two details:
touch go.moddoesn't lead to anything to commit - git doesn't record when you modified it, only what the modifications were.Because of this, on e.g. a brand new
git clone, all files are "the same" age... with some randomness about what ones are newer than others, within e.g. milliseconds. So as far as Make is concerned, a bunch of random files have been modified. If you had makefile rules likeoutput.go: input.go \n do codegen, some of those output files may be newer than their inputs, so they may or may not be re-made.That's pretty clearly wrong. The same kind of thing happens when you change branches - depending on what changed between your before/after commits, those files are now "new".
The way out of this madness is to rely on files that e.g. Git does not modify: these book-keeping files.
So that's what this PR does.