@dsanders11 the failure is owing to a small issue fixed in master recently - rebase and it should be resolved :)

And this looks really exciting! I'll give it a more thorough review come Monday 😁

@codebytere, thanks! I've rebased it on master and added a few changes. Looks like there's still some failures though. Since the flag for the feature is false in the PR, there should be no changes, so I presume the failures are unrelated. That does bring up a question about should I flip the feature on for the PR (and have it flipped back to off before merging) so that the build checks actually use the code. Although if the builds are doing a clean build it won't really exercise it (but would at least show that it doesn't break it).

Looking forward to the more thorough review.

@codebytere, flipped the feature on, the CI builds look good, just a test failure on macos which seems unlikely to be related.

Can we end up with a stale mtime cache that causes difficult-to-diagnose build failures?

Agreed on this concern, and the more nefarious situation where a build works but it's not what you expected it to be. That's why I've initially made it opt-in by defaulting the flag to false, since I didn't want to be responsible for causing cryptic build issues. 😄 However, like you said, the SHA-256 step should make this safe, outside of logic errors in this script.

However, as an additional precaution we should probably delete the mtime-cache.json file after applying it.

This seems reasonable, and something I'd considered during implementation. It is a slight trade-off in the "difficult to debug" direction since it leaves no trace. I think I'll add a flag --preserve-cache for applying so that nothing has to be hacked up if you want to keep the cache around to inspect.

I suspect (2) is not possible, because gclient will always want to reset repos to the version listed in DEPS. I'm not aware of any good solutions in that direction.

I came to the same conclusion, after initially investigating how patches were applied since I wasn't familiar.

RE (2) in theory you could git tag THING when the patches are applied and then do git checkout THING instead of applying all the patches. Not sure how much faster that is though

@MarshallOfSound applying the patches is pretty quick, I don't think it's worth the complexity of creating and managing new refs.

git checkout modifies the files so mtimes wouldn't be preserved anyway.

Made a new commit. Doing some more local testing I was seeing weird results from gclient sync if the pre-DEPS hook exited with an error code. Instead of just stopping the sync there where the error happened it continued on and in doing so seemed to thrash the src tree. That made me weary and so I made the script never exit with an error code other than 0, just logging the error. This also ensures the apply step doesn't stop the sync if the generate step failed, or if --noprehooks was used (which you can currently use as a quick way to disable the feature, although if any future pre-DEPS hooks exist that might not be a good idea).

So, the latest commit changed:

• Better use of argparse for keeping the arguments between the commands separate
• Avoids exiting with a non-zero exit code
• Refactored a little to not apply the mtime until all files had been checked in case something errored out. This hopefully avoids a half-applied situation, although even that situation should be safe.
• Delete the cache after applying (as @nornagon requested)
• Added a set command which is only intended for advanced use, letting you set the mtime for all patched files to a specific value. During testing this let me recover from a sync with the mtime cache feature not in use. Since I know the files haven't changed I could set them back to an mtime before that sync and save myself the hour of rebuilding. Not intended for use by anyone who doesn't know what it does, and it includes a warning as such.

@ckerr, @nornagon, please review the latest commit. Apologies for the thrash, but the no non-zero exit code change was important for robustness. I should be done touching it now that's done.

I'm still 👍 on this PR and am going to use it locally for a few days to see if anything catches fire

I'm still on this PR and am going to use it locally for a few days to see if anything catches fire

I've been moving back and forth between branches and have no problems to report so far!

No Release Notes