Added a FileHasProvider implementation for Unity Version Control #148
Conversation
…stic SCM) Using the cm ls command https://docs.unity.com/en-us/unity-version-control/uvcs-cli/list
|
@microsoft-github-policy-service agree company="Tenstar" |
…ause intermittent errors
src/Common/MSBuildCachePluginBase.cs
Outdated
| return (null, null); | ||
|
|
||
| static string? GetRepoRootInternal(string path) | ||
| static (string?, VersionControl?) GetRepoRootInternal(string path, PluginLoggerBase logger) |
There was a problem hiding this comment.
Should the return value instead be (string, VersionControl)?? They're either both null or neither are null, so the tuple itself being nullable instead of each value seems to express the behavior better.
There was a problem hiding this comment.
Hmm, I couldn't get that to compile when reading the value from the nullable tuple, as in (string, VersionControl) = GetRepoInfo(context, logger);
Instead, I opted for a simpler container class called RepoInfo, code is updated.
src/Common/SourceControl/UnityVersionControl/UnityVersionControl.cs
Outdated
Show resolved
Hide resolved
| } | ||
| else | ||
| { | ||
| _logger.LogMessage($"{file} is missing a hash and will be rehashed."); |
There was a problem hiding this comment.
Just checking since I'm not familiar with unity, is this the real behavior? For untracked files it will return a line with just the file name and nothing else?
There was a problem hiding this comment.
Yes haha, that is the real behaviour as you can see in the picture. Do you think that will affect something?
| StringBuilder? line; | ||
| while ((line = reader.ReadLine()) != null) | ||
| { | ||
| var splitLine = line.ToString().Split('\t'); |
There was a problem hiding this comment.
Can this be refactored to do less allocation? Splitting a string like this allocates an array and new strings.
I imagine this is a somewhat hot path, especially for larger repos, so we should try and make this code as efficient as possible.
There was a problem hiding this comment.
Removed the array allocation from string.Split
|
|
||
| internal Task GitHashObjectAsync(string basePath, List<string> filesToRehash, Dictionary<string, byte[]> filehashes, CancellationToken cancellationToken) | ||
| { | ||
| return Git.RunAsync( |
There was a problem hiding this comment.
It seems weird to fall back to git here. Does Unity not have an equivalent command to use?
There was a problem hiding this comment.
I have not found a suitable command within the cm program that is equivalent. The full list is here https://docs.unity.com/en-us/unity-version-control/uvcs-cli/version-control-cli
| cancellationToken); | ||
| } | ||
|
|
||
| private sealed class UnityVersionContorlLsFileOutputReader : IDisposable |
There was a problem hiding this comment.
Some (all?) of this appears to be copied from the git impl. Can you extract it to for code reuse?
There was a problem hiding this comment.
Extracted out GitHashObjectAsync.
The Reader has a difference in that it doesn't launch a Task to start the parsing. This helped me work around issue where the UnityVersionContorlLsFileOutputReader's reader got Disposed before PopulateAsync was done
|
First off, thank you for your contribution!! Can you comment on the kind of testing you've done? And also attach some logs and/or screenshots as evidence of functionality? |
Hi, thank you, and thank you for the feedback, I will take a look at it this week. Yes, of course! I've only tested with the Local cache with MSBuild version 17.14.23+b0019275e. To figure out #146 I made two brand new VS projects, one static lib and one console app. From there I got my first successful use of the cache and then I added more and more of our internal projects as dependencies to the console app. Once I understood that the solution has to be above its projects I changed our real project to behave the same. Below you can see screenshot of one build populating the cache and the second a clean build on the same changes. 
From there I've gotten it to run after the build agents as a test, its not "live" internally yet. A couple of snags I hit were
Below is a snippet from the initial log from a build using the cache plugin. As you can see we only tweak the log and cache directory settings. Question: Do you recommend ignoring more types of files, like .pngs and stuff? Or are they already not considered by MSBuildCache?
Please let me know if there is some more specific testing you'd like to see or if something so far seems off. I will get back here with some more data from the CI builds when its "live" internally. |
2 participants
This aims to fix #145. At my work, we use Unity Version Control (previously Plastic SCM) https://www.plasticscm.com/ and we would like to be able to use MSBuildCache for our C++ projects. Using the cm ls command https://docs.unity.com/en-us/unity-version-control/uvcs-cli/list, you get similar behaviour as with git ls-files, one big difference being that cm ls can returns hash as a base64 string. For example, the command cm ls -R --format="{path}\t{hash}" would return something like \tzt+fx36QelZDAkae84vEaQ==.