Make project reference lower case in lock file

[jeffkl]

The casing for project names in lock files can change because the value comes from one of two sources:

1. A solution file
2. A project reference

If two project references have different casing, the list is de-duped but in a multi-proc build the order can be different.

The casing in the lock file should probably be .ToLowerInvariant()

{
-      "ClassLibrary1": {
+      "classlibrary1": {
        "type": "Project",
        "dependencies": {
-         "ClassLibrary2": "1.0.0",
+         "classlibrary2": "1.0.0",
        }
      },
}

This should help ensure that the lock file is the same every time.

[rrelyea]

Let's fix for 5.1.
Depending on the risk, we may also merge into 5.0.

[jeffkl]

This is definitely not urgent as the workaround is to unify the casing of project names between all Visual Studio solution files and <ProjectReference /> items in the project tree.

[zivkan]

@jeffkl so I understand, are you asking for this so that git no longer says the file has changed, or is this causing restore failures?

I'm testing some scenarios around projects with the same name but different casing on Linux to see if the proposed change would be a problem. So far it seems to be that there are several other issues that prevent same-name, different case projects working, so writing ToLowerInvariant strings to the lock file shouldn't break any currently working scenario.

[jeffkl]

I haven't seen restore errors but I do see the lock file churning. I keep hitting #7646 so its been hard to test lock files for our use.

[jeffkl]

@zivkan #7889 is much more critical and doesn't have a good workaround for large code bases. I've investigated and found the root cause and have a potential fix here:

NuGet/NuGet.Client@c7a02ff

[zivkan]

I should have chat with you earlier, but I've been out all week with the flu :( I have started some discussions with others about storing the lower case project name in the lock file.

The specific scenario you used when opening the issue, where the case to the csproj is different in the sln and ProjectReference, this will cause one of the two to fail building on Linux, or on Windows or Mac when filesystem case sensitivity is turned on. Therefore there's a question of correctness. If we expect a build to work the same on all platforms, but due to case-sensitivity in a specific repo, does it matter if NuGet changes the case in the lock file? If the team working on the repo fix the issue that prevents building on all platforms, then the lock file casing will no longer change.

Secondly, I'm trying to understand case sensitivity requirements. Should it be allowed to have ProjectA reference ProjectB in one directory and projectb in a different directory? Assume one or both change the assembly name to avoid issues with filesystem case insensitivity when copying all assembles to the output directory. This also brings up the question if it should be allowed to reference two different projects with the same name and case, but are in different directories. This already fails for other reasons, but since the lock file is checked into source control, this has more long lasting implications compared to the files written to the build's intermediate output.

Finally, I'm looking into the performance impact of checking the file system for the file's actual case, either to use in the lock file or maybe to raise a warning. But we need to be very careful about the performance impact.

[jeffkl]

Does the case of the project matter? The value is gotten from MSBuild today which can vary because MSBuild just returns strings. I thought the lock file can be lowercase because its just an index of what projects were referenced. The full transitive closure of packages is already calculated in the other section so from my understanding the list of projects is only needed to help determine if the lock file is out of date. The projects coming in from the dependency graph could be lowercased for comparison with the projects in the lock file, regardless of the casing on disk. Again because the casing is coming from MSBuild anyway, you can't trust the case necessarily.

[zivkan]

I didn't want to break or prevent a scenario that would otherwise work.

Having a quick chat with @nkolev92 about #7889, I think both issues can be resolved at the same time. My understanding is we'll try to align on using assembly name, not project filename, so filesytem casing will not be relevant.

[jeffkl]

It's important to understand that the "name" of the project does not always come from the file on disk.

https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets#L614

By default it is $(MSBuildProjectName), then $(AssemblyName) and finally $(PackageId). The value of MSBuildProjectName value can come from the Solution file or a <ProjectReference />. And there is no guarantee that the casing matches the actual file on disk. You could have 10 projects referencing the same project with 10 different casings and the $(MSBuildProjectName) value could be different for each build. At no point does MSBuild check the casing of the file since it was designed for Windows which has a case insensitive file system. The casing of the project name in the project file should be case-agnostic in my opinion.

[rrelyea]

Discussing this more, we've decided that we'd like to:

1. keep our current behavior (like IDs in asset file) - case preserving, but case insensitive in comparisons.
2. make sure dotnet add reference... and similar scenarios should not lead you to the non-matching casing. @zivkan is going to test and follow up with any issues there.

@anangaur @nkolev92

keeping open while @zivkan researches the 2nd bullet.

[zivkan]

I confirmed that the latest .NET Core 2.2 dotnet add reference uses the case provided on the command line, not the case of the filesystem. I created dotnet/cli#11079 requesting this be changed.

[jeffkl]

@rrelyea I'm worried about the scenario where I changed one reference and the whole file is regenerated and I see a bunch of case differences instead of the single change.

The casing can come from a project-to-project reference, an SLN, or the command-line. It seems like it should be normalized.

[anangaur]

@jeffkl
Essentially its about once vs. consistently showing these casing differences. And I kind of agree with you and vote for making this change once :) which means existing users will see a big change in the lock file one time when they change any dependency since we will shift to normalized casing after this fix. But subsequent changes would not affect the lock file.

@nkolev92 @rrelyea @zivkan I changed my vote 😄 What do you think?

[jeffkl]

Lock files have a version. Perhaps if it exists and its version 1, we keep the old behavior. If it gets created the first time or I delete them all and start over they are version 2 with lowercase IDs. Kinda complicated but might be worth it.

[nkolev92]

I'm worried about the scenario where I changed one reference and the whole file is regenerated and I see a bunch of case differences instead of the single change.

But this again depends on how you keep regenerating your lock file.

It'll only be an issue if you keep alternating between sln file restore and project restore when adding references.
How likely is that?

It's such an edge case but our only option is a big hammer which makes things weird.

[jeffkl]

I'm more talking about a team of people. You use solution files, I use traversal, another person restores a leaf node project. We might all get different casing.

[zivkan]

A restore without changing package or project dependencies doesn't change the lock file (if it does, then there's a bug), so there shouldn't be large churn of the file, only when dependencies change.

Also, projects that are like this aren't cross-platform since they'll have problem on linux, or on Windows or Mac when file system case sensitivity is turned on. Although not all projects care about being xplat compatible.

Finally, package ids in the lock file are cased like in the csproj. If we lower-case the project ids, we should probably lower case the package ids too. But that reduces human readability. While the main purpose of this file is to be read programatically, how human readable should it be?

[jeffkl]

A restore without changing package or project dependencies doesn't change the lock file (if it does, then there's a bug), so there shouldn't be large churn of the file, only when dependencies change.

For me, one of the big draws of lock files is I can see an exact diff of what changed when someone changes a dependency. If there's a bunch of noise because of casing differences, it will lower the value of lock files

Also, projects that are like this aren't cross-platform since they'll have problem on linux, or on Windows or Mac when file system case sensitivity is turned on. Although not all projects care about being xplat compatible.

I do not understand why the casing in the lock files has to match what's on disk. We never use the lock file to look for files on disk right?

Finally, package ids in the lock file are cased like in the csproj. If we lower-case the project ids, we should probably lower case the package ids too. But that reduces human readability. While the main purpose of this file is to be read programatically, how human readable should it be?

It does lower the readability a little since proper casing helps break up words like mypackagea instead of MyPackageA. But some packages break it up with . anyway like microsoft.build.utilities.core. If we could get the casing from disk every time that would be amazing but the perf implications are too great.

[zivkan]

I do not understand why the casing in the lock files has to match what's on disk. We never use the lock file to look for files on disk right?

The casing in the lock file does not have to match what's on disk, that's correct, so if implemented it won't prevent builds on case-sensitive file systems. I'm just saying this is a feature that "only" benefits solutions that have known environment-configuration issues and therefore won't build in environments where that configuration is different.

[jeffkl]

The first repo I tried out lock files hit it. I think its because it has ~500 projects. The teams I interact with have very large repos and will benefit greatly from lock files. But if the lock files are churning a lot they will turn it off.