Cache version information to disk #711
Conversation
|
Hm, I wonder why that test breaks on AppVeyor, but not on my machine. I'll look into it. |
Cache entries are placed in .git\gitversion_cache\ directory.
…ke testing easier.
… to make testing easier.
…us and quite possibly enjoyable task.
…ersion cache file.
… it's easier and more consistent to exclude properties that aren't version variables.
…e` property and make use of it in `VersionAndBranchFinderLoadVersionVariablesFromDiskCache()`.
…ernal and internals visible to GitVersionTask.Tests so we can reset the in-memory cache for testing. Not very elegant, but that's `static` for you. :-/
…m work, since AppVeyor don't seem to agree with local test running in that everything is green. Not trying to fake it may be a better solution anyhow. Ref https://ci.appveyor.com/project/GitTools/gitversion/build/3.3.1-PullRequest.711+28%20(Build%20458)
Sigh. A step in the right direction, at least. Adding a remote to the test repository, then! |
|
This is weird. No other test doing real repository work have any mention of any remotes. How are they working, while my tests fail? @JakeGinnivan do you know? |
| using LibGit2Sharp; | ||
|
|
||
| using NSubstitute; | ||
|
|
||
| public class TestFileSystem : IFileSystem |
There was a problem hiding this comment.
@GeertvanHorrik fyi. Maybe just make the file system stuff internal in Core. The needed abstractions will be different in each app
There was a problem hiding this comment.
Yea, I hoped the abstraction would be useful, but I ended up not using it. Mocking with static classes and methods is nigh impossible.
| @@ -0,0 +1,140 @@ | |||
| #region License | |||
There was a problem hiding this comment.
Doh. ReSharper is a bit too helpful at times. :/
…ith `Repository.Discover()`.
|
@JakeGinnivan I've refactored |
…rs in `SetLoggers()`.
…teCore` and replaced all usage of the former with the latter. Also made `ExecuteCore` non-static.
|
@JakeGinnivan I've refactored deleted However; this, and probably other various changes I've made, are backwards incompatible for someone using GitVersion programmatically. It should not break anything integrating with GitVersionTask or I'm therefore unsure if this warrants a major version increment or not. If it does, I can create a new pull request that preserves API backwards compatibility, but it will be difficult to make the caching work across the task and exe without breaking, since the code paths are different. I'm also still unable to figure out why my tests are failing. |
|
I probably will not get to this one this weekend, hopefully next weekend If it does need a major version bump I will integrate into the v4 branch, otherwise I will manually merge into the current release branch. Will take me some time to review and get my head around all the changes again. My speaking commitments are now all done so I have some free time again :) |
|
@JakeGinnivan Ok. Good to see your comments here again, I hope your talks have gone great! :) Besides the regular code review, what I need you to look at is why the tests are failing on AppVeyor and if the internal changes in method signatures and such is enough to warrant a major version bump. I can also bisect and do another PR without backwards incompatible changes, if you want to. But I think having one code path through both the Looking forward to reading your comments on the code! 😄 |
|
|
||
| var cacheFileName = string.Concat(Path.Combine(cacheDir, cacheKey), ".yml"); | ||
| VersionVariables vv = null; | ||
| if (this.fileSystem.Exists(cacheFileName)) |
There was a problem hiding this comment.
Do we need some sort of cleanup now? Otherwise we will end up with potentially thousands of files in that directory?
Maybe add in a cleanup which deletes everything older than a week?
There was a problem hiding this comment.
Yes, that sounds like a good idea.
There was a problem hiding this comment.
We could put the key into the file, rather than the filename?
There was a problem hiding this comment.
Branch: master
Key: ...
SemVer: ..
...??
There was a problem hiding this comment.
@JakeGinnivan Sure, I can't see why not?
|
I am happy with this. I would like a |
|
I am just going to play devils advocate on this PR now before I merge it. What scenarios do you see where gitversion.exe will be used and the refs will not change. I only use gitversion on the build server and it gets triggered when the VCS changes, I can't see I will get a benefit from this PR. Wondering if maybe caching on exe should be opt in, rather than opt out? |
|
@JakeGinnivan With Caching on the Task should imho not be opt-in, since this speeds up the build quite significantly. I also can't see that this breaks anything other than not being backwards compatible on the code level. The tests of course need to be fixed for AppVeyor, though. I really need help to figure out what's going on there. |
This PR adds support for caching the version information to a YAML file on disk, so successive builds without changes in Git can be sped up. In local tests, the build time is cut in half with this PR in place.
The cache file's name is keyed by the path to the
.gitdirectory, the canonical name of the repository'sHEAD, the SHA of the repository'sHEADtip and the last modified date of the.git/refsdirectory. This should ensure that the file name changes every time you do something in Git that will affect the version number.I've had to do quite a bit of refactoring to be able to test this functionality. Most mentionable are the static classes
GitDirFinder,RepositoryLoaderandDirectoryDateFinderwhich I've deleted and whose methods I've moved intoIFileSystemso they can be mocked.To simplify mocking, I've also added a reference to NSubstitute. I hope this is OK. Mocking LibGit2Sharp's
IRepositoryinterface would otherwise be a huge PITA.