-
Notifications
You must be signed in to change notification settings - Fork 676
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
Repeatable build using Project lock file implementation #2342
Conversation
74b458c
to
e19ac3d
Compare
@NuGet/nuget-client 🔔 This is ready to be reviewed... Just skip the test part which i'm still working on. |
e19ac3d
to
2fc37b3
Compare
df760da
to
0b0e8bd
Compare
@DoRonMotter suggested to add the team alias but explicitly add only those individuals who should really take a look. So I've updated the reviewers list accordingly. @NuGet/nuget-client @rrelyea 🔔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good. Bunch of comments, most of them to improve readability and have a consistent code style. I'm concern that we have a LockFile
class that refers to the assets file and a NuGetLockFile
that refers to the actual lock file, this will make the code REALLY confusing. I think we should rename the current LockFile
to have a better naming (i.e. AssetsFile
) and then call this new class LockFile
.
_lockFileVersion = lockFileVersion; | ||
} | ||
|
||
public NuGetLockFile CreateNuGetLockFile(LockFile assetsFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like keeping the current assestsFile class as LockFile
and creating a new NuGetLockFile
class for this is REALLY confusing. I would argue in favor of renaming LockFile
to AssetsFile
in another PR first and then naming this LockFile
instead of NuGetLockFile
. Therefore also this class can be renamed to LockFileBuilder
because we don't need to prepend NuGet
to our class names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we call NuGetLockFile RepeatableBuildLockFile or something more descriptive? I do agree that it's incredibly confusing to see LockFile at someplace and NuGetLockFile in others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be difficult to rename the current lock file classes, as they are used heavily in the build targets.
Maybe in 5.0, but even then we'd likely have to take the initiative to fix the build targets ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should discuss the naming concerns offline.
We have the same issue with the config changes.
We need to address both at an uber level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkolev92 so you are suggesting that we should just keep piling on the tech debt? Honestly, I can see how someone looking at this for the first time get really confused about the fact that there is a LockFile
class that has nothing to do with the repeatable build lock file. I would prefer to fix that first before adding more debt that will later be more difficult to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suggesting that we should have a naming concerns discussion and that this specific class LockFile is unlikely to change in 15.9 because the build targets are using it.
I'd be up for cleaning up the APIs in 5.0/16.0, but we have to commit to doing it rather than postpone it into perpetuity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @nkolev92 that we can discuss the naming concerns offline and come to a conclusion.
But just to add my point of view here, we have some shitty naming conventions across our code base and I certainly wouldn't want to pile on top of that but we also have to realize that we're not in a re-factoring phase here and have the liberty to spend weeks and months on that. What we can do here is have less confusing names and better class summaries which clearly differentiates.
Now about this new lock file, then may I propose PackagesLockFile
class name since the file name itself is packages.lock.json
so it should be convenient to understand and have the clear summary to indicate that this class represents repeatable build packages.lock.json
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline individually with most of you, I'll go ahead with PackagesLockFile
with appropriate summary description and for all other subsequent classes. And also logged a tracking issue# NuGet/Home#7207 to do some refactoring which we can discuss when we plan for dev16.
|
||
foreach (var target in assetsFile.Targets) | ||
{ | ||
var nuGettarget = new NuGetLockFileTarget() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as comment above, there shouldn't be a need to prepend NuGet
to the class name.
Also nit: rename nuGettarget
to nuGetTarget
for proper casing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not een sure what the proper casing is there lol.
NuGet*bla is confusing for sure.
As mentioned in the above comment, I'd prefer an offline pros/cons discussion for our naming strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please refer the above comment
var framework = assetsFile.PackageSpec.TargetFrameworks.FirstOrDefault( | ||
f => EqualityUtility.EqualsWithNullCheck(f.FrameworkName, target.TargetFramework)); | ||
|
||
var lilbraries = target.Libraries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: libraries
{ | ||
var onlyTFM = assetsFile.Targets.First(t => EqualityUtility.EqualsWithNullCheck(t.TargetFramework, target.TargetFramework)); | ||
|
||
lilbraries = target.Libraries.Where(lib => !onlyTFM.Libraries.Any(tfmLib => tfmLib.Equals(lib))).ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider setting var libraries
to null
and then doing an if\else
statement to avoid possibly loading the libraries
variable twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're not actually loading it twice, first is the assignment statement similar to assigning to null and then update it if required. Do you see anything wrong with that?
Dependencies = library.Dependencies | ||
}; | ||
|
||
var framework_dep = framework.Dependencies.FirstOrDefault( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that you used FirstOrDefault
, framework
could be null
and this would give an exception.
Also nit: rename framework_dep
to frameworkDependency
, we should not be mixing CamelCase and snake_case in our code, and c# uses CamelCase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
framework cannot be null since we can't have a framework inside lock file targets which doesn't exists in packageSpec. But if we still want to be safeguard then i can handle that as well.
And agree with naming convention thing, but personally i dont care too much about block level local variables, rather be more concerned about class or method level variables. But I'll change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it cannot be null then use First
which will throw an exception if the value doesn't exist.
{ | ||
// Arrange | ||
var sourceRepositoryProvider = TestSourceRepositoryUtility.CreateSourceRepositoryProvider( | ||
new List<Configuration.PackageSource>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please add a tab for correct indentation.
{ | ||
// Arrange | ||
var sourceRepositoryProvider = TestSourceRepositoryUtility.CreateSourceRepositoryProvider( | ||
new List<Configuration.PackageSource>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please add a tab for correct indentation.
{ | ||
// Arrange | ||
var sourceRepositoryProvider = TestSourceRepositoryUtility.CreateSourceRepositoryProvider( | ||
new List<Configuration.PackageSource>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please add a tab for correct indentation.
{ | ||
// Arrange | ||
var sourceRepositoryProvider = TestSourceRepositoryUtility.CreateSourceRepositoryProvider( | ||
new List<Configuration.PackageSource>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please add a tab for correct indentation.
|
||
namespace NuGet.PackageManagement.VisualStudio.Test | ||
{ | ||
public class TestVSProjectAdapter : IVsProjectAdapter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider using =>
in every one-line method in this class, to create inline function declarations.
Also, should this class be internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes to internal class and no to inline function
@@ -57,6 +57,9 @@ public sealed class VsSolutionRestoreService : IVsSolutionRestoreService, IVsSol | |||
private const string TreatWarningsAsErrors = nameof(TreatWarningsAsErrors); | |||
private const string WarningsAsErrors = nameof(WarningsAsErrors); | |||
private const string NoWarn = nameof(NoWarn); | |||
private const string RestorePackagesWithLockFile = nameof(RestorePackagesWithLockFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these 3 properties be encapsulated by a single class like RestoreLockFileProperties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm good suggestion.
}; | ||
|
||
var framework_dep = framework.Dependencies.FirstOrDefault( | ||
dep => PathUtility.GetStringComparerBasedOnOS().Equals(dep.Name, library.Name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is framework dependency an actual path? shouldn't it always be a case insensitive comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is dependency name and would be case sensitive for linux machines.
e5b2e9e
to
2469687
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm halfway through the PR.
I will continue tomorrow, here's the initial feedback.
@@ -57,6 +57,9 @@ public sealed class VsSolutionRestoreService : IVsSolutionRestoreService, IVsSol | |||
private const string TreatWarningsAsErrors = nameof(TreatWarningsAsErrors); | |||
private const string WarningsAsErrors = nameof(WarningsAsErrors); | |||
private const string NoWarn = nameof(NoWarn); | |||
private const string RestorePackagesWithLockFile = nameof(RestorePackagesWithLockFile); | |||
private const string NuGetLockFilePath = nameof(NuGetLockFilePath); | |||
private const string FreezeLockFileOnRestore = nameof(FreezeLockFileOnRestore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please unify the properties/switches in msbuild in both specs?
The code matches https://github.com/NuGet/Engineering/blob/master/Client.Specs/RepeatableBuildProjectLockFile.md, but is different from https://github.com/NuGet/Home/wiki/Enable-repeatable-package-restore-using-lock-file#extensibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anangaur is working on unifying it in his spec. And if he change anything then i'll reflect that.
public class NuGetLockFileBuilder | ||
{ | ||
private readonly int _lockFileVersion; | ||
public NuGetLockFileBuilder(int lockFileVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format: add a new between the field and the constructor.
_lockFileVersion = lockFileVersion; | ||
} | ||
|
||
public NuGetLockFile CreateNuGetLockFile(LockFile assetsFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should discuss the naming concerns offline.
We have the same issue with the config changes.
We need to address both at an uber level.
|
||
foreach (var target in assetsFile.Targets) | ||
{ | ||
var nuGettarget = new NuGetLockFileTarget() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not een sure what the proper casing is there lol.
NuGet*bla is confusing for sure.
As mentioned in the above comment, I'd prefer an offline pros/cons discussion for our naming strategy.
@@ -0,0 +1,102 @@ | |||
using System; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the usings (pretty sure some are unused, like the nuget.frameworks one) and add a header.
if (_request.ExistingLockFile != null) | ||
{ | ||
newAssetsFile = _request.ExistingLockFile.Clone(); | ||
newAssetsFile.LogMessages = logs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I think you need to replay the old warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me check with Ankit on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're replaying the warning only to replace them again, why?
Maybe I'm missing something.
lockFile: newAssetsFile, | ||
previousLockFile: _request.ExistingLockFile, | ||
lockFilePath: _request.ExistingLockFile?.Path, | ||
cacheFile: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, you have the cache file already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responded above, would like to keep it this way until there is a valid scenario instead to pass cache file details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responded above for the reasoning.
@@ -313,12 +438,36 @@ private bool VerifyAssetsFileMatchesProject() | |||
return (_request.ExistingLockFile != null && pathComparer.Equals( _request.ExistingLockFile.PackageSpec.FilePath , _request.Project.FilePath)); | |||
} | |||
|
|||
private KeyValuePair<CacheFile, bool> EvaluateCacheFile() | |||
private bool ValidatePackages(NuGetLockFile lockFile, LockFile assetsFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was self-explanatory but open for suggestions. How about ValidatePackagesSha
or ValidatePackagesSha512
etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that might work. imho the current name is too simple.
I'm not against making it even more verbose such as ValidateLockFilePackagesShaMatchesAssetsFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah that's toooooooo verbose. I'll go with ValidatePackagesSha
@@ -157,23 +172,26 @@ public virtual async Task CommitAsync(ILogger log, CancellationToken token) | |||
|
|||
BuildAssetsUtils.WriteFiles(buildFilesToWrite, log); | |||
|
|||
if (result.LockFile == null || result.LockFilePath == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is adding a check that wasn't there before.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err because that was missing and there was no test covering that... we had that for tool restore but not for actual restore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of this?
This never happens because otherwise we would have had a runtime error.
Maybe the correct approach is to null check in the constructor of the result.
Here it just hides potential problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh never mind.
I finally got this because of the comment.
Basically you're covering this case.
https://github.com/NuGet/NuGet.Client/pull/2342/files#diff-ae3ea71e74915c8f0634a376599d6b87R249
Similar to what I recommended in other places.
I'd rather have an additional flag that indicates the scenario rather than rely on nulls as valid scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having LockFile as null when we've bailed out the restore completely make sense to me and the right approach. Adding an additional flag for it, is what doesn't make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My problem with this is that it promotes bad practice, where you don't know if things are null and when they are null and what does it mean for them to be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm out of arguments for this... I sincerely dont follow your concerns over bad practice for this. It's as simple as any other null check in an api to before consuming anything.
Lets talk about this offline and finalize it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline, we're settled on this.
@@ -293,7 +293,8 @@ public static async Task<RestoreSummary> CommitAsync(RestoreResultPair restoreRe | |||
} | |||
|
|||
// Remote the summary messages from the assets file. This will be removed later. | |||
var messages = restoreResult.Result.LockFile.LogMessages.Select(e => new RestoreLogMessage(e.Level, e.Code, e.Message)); | |||
var messages = restoreResult.Result.LockFile?.LogMessages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because with packages lock file, there could be a case where there was no existing assets file and we fail restore because packages lock file is different than project file and user can denied updating packages lock file with restore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to the above comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to all of you for taking your time to review this. I've tried to respond to each and every comment and will follow up with offline discussion if required to clarify some of the doubts.
lockFile: newAssetsFile, | ||
previousLockFile: _request.ExistingLockFile, | ||
lockFilePath: _request.ExistingLockFile?.Path, | ||
cacheFile: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responded above, would like to keep it this way until there is a valid scenario instead to pass cache file details
@@ -313,12 +438,36 @@ private bool VerifyAssetsFileMatchesProject() | |||
return (_request.ExistingLockFile != null && pathComparer.Equals( _request.ExistingLockFile.PackageSpec.FilePath , _request.Project.FilePath)); | |||
} | |||
|
|||
private KeyValuePair<CacheFile, bool> EvaluateCacheFile() | |||
private bool ValidatePackages(NuGetLockFile lockFile, LockFile assetsFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was self-explanatory but open for suggestions. How about ValidatePackagesSha
or ValidatePackagesSha512
etc...
{ | ||
CacheFile cacheFile; | ||
var noOp = false; | ||
|
||
var newDgSpecHash = NoOpRestoreUtilities.GetHash(_request); | ||
var newDgSpecHash = dgSpec.GetHash(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it isn't... come out of NoOpRestoreUtilities
and then think, we're generating dgSpec
which can and should be used down further if required... Generating dgSpec
isn't entirely tied up with NoOp
check... it's just that NoOp check is the first which needs to generate it.
@@ -163,5 +163,7 @@ public class RestoreRequest | |||
public SignedPackageVerifierSettings SignedPackageVerifierSettings { get; set; } = SignedPackageVerifierSettings.GetDefault(); | |||
|
|||
public Guid ParentId { get; set;} | |||
|
|||
public bool IsRestore { get; set; } = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the current problem, We only need to differentiate between "restore" and install/ update hence a boolean works fine. I'll talk to Zhi if he wanted to do something similar and how that can be clubbed here.
/// <summary> | ||
/// NuGet lock file path | ||
/// </summary> | ||
public string NuGetLockFilePath { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested above, how about PackagesLockFilePath
?
return target; | ||
} | ||
|
||
private static LockFileDependency ReadTargetDependency(string property, JToken json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as I responded above
{ | ||
path = Path.Combine(project.BaseDirectory, "packages." + project.RestoreMetadata.ProjectName.Replace(' ', '_') + ".lock.json"); | ||
|
||
if (!File.Exists(path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File.Exists is case insensitive and it can't be confusing.
I didn't understand what you mean by this?
Also, why should we use ProjectPath
when there is already ProjectName
? Are there cases where ProjectPath is set but not ProjectName in restore metadata? if not, then I'll lean towards using ProjectName and may be fixed that at other places going forward...
/// <summary> | ||
/// Set when customer wants to opt into packages lock file | ||
/// </summary> | ||
public string RestorePackagesWithLockFile { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are actually three cases of this property - Null, true, and false. Most cases Null and false are treated same except when you also have an explicit packages.lock.json
file... in that case null will be accepted but false is an error case. This is explained in the spec when someone set these properties.
public IProjectSystemCapabilities Capabilities { get; } = Mock.Of<IProjectSystemCapabilities>(); | ||
public T GetGlobalService<T>() where T : class | ||
{ | ||
throw new NotImplementedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above I'm not a fan of inline functions... I haven't tried to change anything in existing apis but new apis have proper method signature
|
||
namespace NuGet.PackageManagement.VisualStudio.Test | ||
{ | ||
public class TestVSProjectAdapter : IVsProjectAdapter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes to internal class and no to inline function
warningsAsErrors: _vsProjectAdapter.WarningsAsErrors), | ||
RestoreLockProperties = new RestoreLockProperties( | ||
await _vsProjectAdapter.GetRestorePackagesWithLockFileAsync(), | ||
await _vsProjectAdapter.GetNuGetLockFilePathAsync(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to change these as well to align with the eventual NuGetLockFile name change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these are fine since the MSBuild property name itself is NuGetLockFilePath
@@ -62,6 +62,8 @@ public class RestoreArgs | |||
|
|||
public Guid ParentId { get; set; } | |||
|
|||
public bool IsRestore { get; set; } = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this an enum which actually identifies what operation it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it was discussed in earlier comments conversation for the same topic with @nkolev92, currently I only need to differentiate between restore n rest which is why a simple bool. There is no other specific ask to differentiate between other operations at this time. Also, just adding a enum here wouldn't do anything since we'll need to plump it through all the way from UI context to differentiate between Install or Update...
@zhili1208 was looking at this while working for NU1605 issue but realized this will be a bigger ask so we'll add enum or operation types when take up that work which is not planned yet and could take years to become reality.
}; | ||
|
||
var libraryLookup = assetsFile.Libraries.Where(e => e.Type == LibraryType.Package) | ||
.ToDictionary(e => new PackageIdentity(e.Name, e.Version)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need a dictionary if there is no key value pair? convert the IEnumerable to a HashSet instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look closely, that is actually a dictionary where key
is PackageIdentity
and value
is LockFIleLibrary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2nd of the first pass.
|
||
var path = project.RestoreMetadata.RestoreLockProperties.NuGetLockFilePath; | ||
|
||
if (string.IsNullOrEmpty(path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In whcih case will this be null?
If it's only tests, i'd really try to make sure that the tests populate it as often as possible.
That way you case you can just call project.RestoreMetadata?.RestoreLockProperties.NuGetLockFilePath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When user doesn't set this NuGetLockFilePath
property explicitly and instead wants to use the default file name n path then it will be null. This is not for tests. You can find more details in specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's restore metadata, you should generate it at the beginning similar to what we do with the assets and cache files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again not able to understand your concerns here as well... may be we speak different language. its to know the lock file path when we start to evaluate it now i dont understand what you mean by at the beginning...?
Again lets discuss offline
|
||
var restorePackagesWithLockFile = _request.Project.RestoreMetadata?.RestoreLockProperties.RestorePackagesWithLockFile; | ||
|
||
if (!MSBuildStringUtility.IsTrueOrEmpty(restorePackagesWithLockFile) && File.Exists(nuGetLockFilePath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style:
The culprit isn't this PR solely, but this method is now around ~400 lines.
You can refactor out some smaller methods to make it more readable.
Additionally I think there are too many exit points in this method (~4 different places that return, at least one can be minimized)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to break it down further, but the problem was this method already had NoOp and actual restore. And this new code has be in between some where using or forwarding most of the existing stuff.
// validate package's SHA512 | ||
_success &= ValidatePackagesSha(nuGetLockFile, assetsFile); | ||
|
||
// clear out the existing lock file so that we don't over-write the same file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this pattern.
The restore result should contain the up to date latest value of the lock file not a null if it wasn't updated.
If you're worried about the comparison (and I'd even say that's the right approach), I'd add another flag saying if the lock file has been updated and whether it needs writing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The restore result should contain the up to date latest value of the lock file not a null if it wasn't updated.
Restore result is only used to commit files afterwards and in this case we've used the existing file to perform restore. There is no reason to keep persisting this value. But may be i'm missing something so please enlighten me with your concerns here or we can talk about this offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think about it from an API perspective.
I run 2 consecutive restores.
Nothing get overriden, no new cache, no new assets file, no new lock file.
Yet the API returns me 2 different results.
One that contains the lock file another one that doesn't.
That's bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You run 2 restores -
- first restore:
a. fresh restore - so nothing already exists n a new cache file, assets file and lock file gets created and returned.
b. noop restore - everything was there so new lock file and neither was returned.
c. restore changed - there was cache file n assets file but no lock file. So this restore generated a new cache file, assets file and lock file which are being returned.
……………. similarly try any other scenarios - second restore: there are always these files so no new cache file, no assets file n no lock file so nothing should be returned. But unfortunately we still return cache file and assets file and I don't want that unfortunate thing with lock file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not an unfortunate thing.
It's by design.
Let's add this to the list of topics to discuss offline :)
{ | ||
CacheFile cacheFile; | ||
var noOp = false; | ||
|
||
var newDgSpecHash = NoOpRestoreUtilities.GetHash(_request); | ||
var newDgSpecHash = dgSpec.GetHash(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to https://github.com/NuGet/NuGet.Client/pull/2342/files#r210479950.
You already have the dg spec on the requst. Use that one.
You don't need to generate it.
That dg spec is being cloned there because it's being modified for the no-op dg spec hashing.
@@ -157,23 +172,26 @@ public virtual async Task CommitAsync(ILogger log, CancellationToken token) | |||
|
|||
BuildAssetsUtils.WriteFiles(buildFilesToWrite, log); | |||
|
|||
if (result.LockFile == null || result.LockFilePath == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of this?
This never happens because otherwise we would have had a runtime error.
Maybe the correct approach is to null check in the constructor of the result.
Here it just hides potential problems.
return true; | ||
} | ||
|
||
return PathUtility.GetStringComparerBasedOnOS().Equals(Id, other.Id) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package id is case insensitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for linux... packageA and packagea are treated different packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take my words back, it's case intensive
@@ -196,7 +200,8 @@ public ProjectRestoreMetadata Clone() | |||
Sources = Sources?.Select(c => c.Clone()).ToList(), | |||
TargetFrameworks = TargetFrameworks?.Select(c => c.Clone()).ToList(), | |||
Files = Files?.Select(c => c.Clone()).ToList(), | |||
ProjectWideWarningProperties = ProjectWideWarningProperties?.Clone() | |||
ProjectWideWarningProperties = ProjectWideWarningProperties?.Clone(), | |||
RestoreLockProperties = RestoreLockProperties?.Clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the tests for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping.
Add tests for the cloning.
}; | ||
|
||
var framework_dep = framework?.Dependencies.FirstOrDefault( | ||
dep => PathUtility.GetStringComparerBasedOnOS().Equals(dep.Name, library.Name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be case insensitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at where this is coming from, it's from LibraryRange.
LibraryRange equals compares the name as case insensitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As i mentioned above, package name is case sensitive for linux and when we're enabled cross platforms we should take care of that. Currently we dont have consistent code across classes which is why we end up in comparison issues as well. But most people don't create multiple packages only difference in casings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked the origin, and indeed it's case insensitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
.ToList(); | ||
|
||
// add lock file libraries into RemoteWalkContext so that it can be used during restore graph generation | ||
contextForProject.LockFileLibraries.Add(new LockFileCacheKey(target.TargetFramework, target.RuntimeIdentifier), libraries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason why you need a mapping for the libraries.
You only need a set of all the libraries. You never make use of the mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this mapping to resolve dependencies which happens in ResolverUtility
} | ||
|
||
public static async Task<GraphItem<RemoteResolveResult>> FindLibraryEntryAsync( | ||
LibraryRange libraryRange, | ||
NuGetFramework framework, | ||
string runtimeIdentifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the runtime identifier per the other comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do because we're being smart enough to not just copy all target graphs across tfm and tfm+rid in new packages.lock.json
file rather tfm+rid will only have the changed dependencies in tfm+rid.
So when we come here to resolve all dependencies for tfm+rid, we need to understand the rid in order to get the corresponding dependency.
Go through the code changes of ResolverUtility
if you still dont understand then let me know i'll try to add more comments there to make it understandable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression that we don't need them and you never truly use them.
I'll bug you offline if I reach the same conclusion when I go over it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this can be changed.
Let's discuss offline.
return MSBuildStringUtility.IsTrue(value); | ||
} | ||
|
||
public async Task<bool> IsReevaluateNuGetLockFileAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this correctly...this would allow users to overwrite perpetually correct?
Is that something we really want in Visual Studio?
Furthermore I don't remember us discussing and agreeing on this at any point, nor is it in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to support --reevaluate
flag with dotnet.exe this is not going to be an explicit property available to be set which is why its not documented in the spec but you can read about --reevaluate
there.
And just to explain, this flag is to reevaluate restore graph even if you had a lock file in order to support floating versions or ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that, this flag should be similar to RestoreForce.
Used in commandline and the restore task but not in VS.
Then why are you reading it in Visual Studio? This is my biggest concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it will be there in visual studio as well in form of a UI gesture n i dont want two different code paths for the same option as we're doing it for force...
we can discuss this offline as well.
0239ec2
to
22054ce
Compare
@NuGet/nuget-client addressed all the comments so far and tried to make it more readable n understandable. |
@jainaashish There are couple of comments from the previous review that I followed up on. Let me know when you've gone through those and I can do another top to bottom pass. |
@nkolev92 it seems right thing would be to discuss some of your concerns offline and resolve. I'm WFH today so lets do that on monday. |
@NuGet/nuget-client Team, there is one open item regarding - https://github.com/NuGet/NuGet.Client/pull/2342/files#diff-ae3ea71e74915c8f0634a376599d6b87R290 which needs to be discussed and finalize. I don't agree to return the packages lock file instance as part of |
@jainaashish can the telemetry and any future additions be in a separate PR? |
@nkolev92 No more :) actually telemetry change was pretty small and made sense to include as part of this PR to let basic telemetries flow... |
9148c22
to
63e2d23
Compare
@NuGet/nuget-client 🔔 |
RestoreLockProperties = new RestoreLockProperties( | ||
await _vsProjectAdapter.GetRestorePackagesWithLockFileAsync(), | ||
await _vsProjectAdapter.GetNuGetLockFilePathAsync(), | ||
await _vsProjectAdapter.IsRestoreLockedAsync()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be repeating the same conversation that we've had before, but do we really want this?
What's the precedence order if you use a reevaluate switch from the UI?
I'm not arguing for or against yet, I'm just trying to understand the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is RestoreLockedMode
property which is to restrict restore updating lock file. This will primarily be for CI machines so that restore doesn't update lock file but dev boxes should always allow which is why default is allowed. But if this property is set even on dev boxes then we have to respect that, hence this code.
This flag is mentioned in the spec with explanation. If you think this is not clear then may be we can ask Anand to make it more clear. - https://github.com/NuGet/Home/wiki/Enable-repeatable-package-restore-using-lock-file
@@ -119,7 +119,8 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
RestoreRecursive="$(RestoreRecursive)" | |||
RestoreForce="$(RestoreForce)" | |||
HideWarningsAndErrors="$(HideWarningsAndErrors)" | |||
Interactive="$(NuGetInteractive)"/> | |||
Interactive="$(NuGetInteractive)" | |||
ReevaluateRestoreGraph="$(ReevaluateRestoreGraph)"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec: This is not documented anywhere in the specs.
This is the reevaluate lock file, hose everything (well almost) right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the implementation part of --reevaluate
flag passed with dotnet.exe restore
Since this is not explicitly allowed or needed for msbuild hence not mentioned in the spec.
And yes, it just skip existing lock file and re-evaluate all the dependencies.
@@ -154,8 +154,7 @@ private static ExternalProjectReference GetExternalProject(PackageSpec rootProje | |||
// Project.json is special cased to put assets file and generated .props and targets in the project folder | |||
RestoreOutputPath = project.PackageSpec.RestoreMetadata.ProjectStyle == ProjectStyle.ProjectJson ? rootPath : project.PackageSpec.RestoreMetadata.OutputPath, | |||
DependencyGraphSpec = projectDgSpec, | |||
MSBuildProjectExtensionsPath = projectPackageSpec.RestoreMetadata.OutputPath, | |||
ParentId = restoreArgs.ParentId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the ParentId removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is being done through ApplyStandardProperties
<comment>{0} - package id and version</comment> | ||
</data> | ||
<data name="Error_InvalidLockFileInput" xml:space="preserve"> | ||
<value>Invalid restore input where RestorePackagesWithLockFile property is set to false and also exist a packages lock file at {0}.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping here.
@@ -329,6 +329,17 @@ private static ProjectRestoreMetadata GetMSBuildMetadata(PackageSpec packageSpec | |||
msbuildMetadata.ProjectWideWarningProperties = new WarningProperties(warnAsError, noWarn, allWarningsAsErrors); | |||
} | |||
|
|||
// read NuGet lock file msbuild properties | |||
var restoreLockProperties = rawMSBuildMetadata.GetValue<JObject>("restoreLockProperties"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know you are following a pattern here, but having a constant would help avoiding weird bugs like case inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to the author who is adding new property needs to make sure to use the correct casing, even with constant, we can fault into that issue. Also, it will be little weird with constant since then it will be something like:
public static readonly string RestoreLockProperties = "restoreLockProperties";
@@ -0,0 +1,75 @@ | |||
using System; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing header
@@ -0,0 +1,90 @@ | |||
using System; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing header
@@ -19,6 +19,21 @@ | |||
<ProjectReference Include="$(NuGetClientsSrcDirectory)NuGet.CommandLine\NuGet.CommandLine.csproj" /> | |||
</ItemGroup> | |||
|
|||
<Choose> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get what this change achieves.
Seems unnecessary as I don't see any functional differences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont know why it changed this. But will revert as part of squash n merge.
@@ -196,7 +200,8 @@ public ProjectRestoreMetadata Clone() | |||
Sources = Sources?.Select(c => c.Clone()).ToList(), | |||
TargetFrameworks = TargetFrameworks?.Select(c => c.Clone()).ToList(), | |||
Files = Files?.Select(c => c.Clone()).ToList(), | |||
ProjectWideWarningProperties = ProjectWideWarningProperties?.Clone() | |||
ProjectWideWarningProperties = ProjectWideWarningProperties?.Clone(), | |||
RestoreLockProperties = RestoreLockProperties?.Clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping.
Add tests for the cloning.
} | ||
|
||
public static async Task<GraphItem<RemoteResolveResult>> FindLibraryEntryAsync( | ||
LibraryRange libraryRange, | ||
NuGetFramework framework, | ||
string runtimeIdentifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this can be changed.
Let's discuss offline.
744b763
to
75ea2fc
Compare
@nkolev92 Added missing headers and a cloning test. |
generating/ updating project lock file consuming project lock file during restore in VS Validation of package's sha512 and added error messages as appropriate Test infra for legacy PackageReference func tests Base workflow is completed Added telemetry events for project lock file feature
75ea2fc
to
bb6c399
Compare
generating/ updating project lock file consuming project lock file during restore in VS Validation of package's sha512 and added error messages as appropriate Test infra for legacy PackageReference func tests Base workflow is completed Added telemetry events for project lock file feature
This is Repeatable build using Project lock file implementation and contain following parts:
Spec - https://github.com/NuGet/Home/wiki/Enable-repeatable-package-restore-using-lock-file
Technical Spec - https://github.com/NuGet/Home/wiki/Repeatable-build-using-lock-file-implementation