Ignore pacakge with unnormalized version #2251
Conversation
|
Yes, change in the PackageRepository was the first though. But then we agreed on Friday that we'd better limit the scope of this change to the dnu restore only. There are more things depends on PackageRepository. |
|
@troydai I prefer that change. It should only affect restore because PackageRepository has 2 code paths. With and without the lock file. |
|
All right. |
|
@davidfowl better? |
|
@troydai Much better! The comment needs some work |
|
Can you add a test? We do have restore tests ping @ChengTian |
|
Yes, tests will be added. I'll work with @ChengTian |
| { | ||
| // For a non-http match, if the OriginalVersion string is not normalized that means name of the folder which contains | ||
| // the package is not a normalized string. It will cause trouble for file searching in later stage. By invalidate this | ||
| // match, it ensures the package will be reinstalled under a correct folder. This change ensures a package installed |
|
Use this as an example of |
|
Updated. Test Added. |
| public void DnuRestore_ReinstallsPackageWithNormalizedVersion(string flavor, string os, string architecture) | ||
| { | ||
| var runtimeHomePath = _fixture.GetRuntimeHomeDir(flavor, os, architecture); | ||
| using (var tempDir = new DisposableDir("/tmp/troy.temp", false)) |
There was a problem hiding this comment.
You're not actually hardcoding a temp directory right? Why isn't this random?
There was a problem hiding this comment.
Ah, shouldn't have it here. It's for local testing. Gonna remove it.
|
⌚ |
| arguments: $"{projectDir} -s {_fixture.PackageSource} --packages {packagesDir}"); | ||
|
|
||
| // rename package folder to an unnormalized string | ||
| Directory.Move(Path.Combine(packagesDir, "alpha", "0.1.0"), |
There was a problem hiding this comment.
Is it possible that the folder can be normalized but the sha file and nupkg inside isn't or that that not possible
There was a problem hiding this comment.
It's actually not possible. Either both normalized or not. https://github.com/aspnet/dnx/blob/dev/src/Microsoft.Framework.PackageManager/Utils/NuGetPackageUtils.cs#L30
But the simplified model used by test is sufficient because the version of a package on disk is parsed from it's folder name: https://github.com/aspnet/dnx/blob/dev/src/Microsoft.Framework.Runtime/NuGet/Repositories/PackageRepository.cs#L102.
|
⌚ |
|
|
does this also fix the issue for publish? An issue we just filed on this
|
I'll try out |
|
This won't fix that issue though. It needs to normalize the version in PublishCommand. It doesn't use I'll merge this one first then address issue #2267 |
Intend to address #2240.
It just test the water. I'll add more test cases.