C#/C++: Convert C# code to use paket package manager

[criemen]

This PR converts our C# codebase to use the paket package manager.

Why am I doing this?

Bazel's rules_dotnet require the use of paket to interface with nuget dependencies. Therefore, we need to maintain a paket.dependencies and paket.lock file for our (future) bazel build.

What are the design choices I made?

Integrated paket into our dotnet CLI build

We could, in theory, keep the existing solution files as-is, but then we'd have to duplicate our external dependencies in two disjoint systems. I don't believe that's a good way forward, and we'd risk our external dependencies going slightly out-of-sync. As we're currently using the dotnet solution files to build an extractor pack for some of our CI, as well as for local development/debugging, I don't think having (potential) differences between those builds is advisable.

One lockfile for all C# code

You can see that the dependency list and lockfile is in the top-level folder. This is because paket requires those files to be in the top-level folder of the "project", i.e. the common folder of all sub-projects. You'd think that's ql/csharp, but as we have C# code in ql/cpp (namely, the Windows autobuilder) that depends on ql/csharp code, I had to move the files to the top level.
An alternative might be to split up the paket dependency files into two, one file for C++, and one for C#. As the C++ projects depend on some of the C# projects, though, that imo made less sense. I've also not explored how the Bazel integration would work if we have two disjoint lock files, but a shared dependency tree.

What are the drawbacks?

I believe the main drawback is having to run dotnet tool restore before dotnet restore/dotnet build will work. Unfortunately that's a dotnet limitation. There'll be a good error message from the dotnet tooling, though, telling you to run dotnet tool restore to install paket if you haven't.
Also, all build/CI scripts have to be adjusted to call dotnet tool restore before attempting to build our code (this is mainly relevant until we switch over to a Bazel-based build).

What are the advantages?

Deduplication of version numbers

Currently, we've duplicated the version numbers of all direct dependencies in all projects that reference those dependencies. Now, with paket, we have a single place (the paket.dependencies file) that lists all our external dependencies version, without (by accident) introducing the same dependency at multiple versions.

Reproducible builds with a lockfile

By using a lockfile, the entire set of transitive dependencies is fixed. In practice, I believe this was not a big problem before, as nuget (to my big surprise) always chooses the minimal version number for all transitive dependencies that satisfy the version constraints in dependency resolution. That way, a newly published package version will not start randomly breaking builds, as we know it from other ecosystems that go for the maximum version that satisfies all constraints.
paket supports both dependency resolution algorithms, and makes taking the maximum version safe by providing lockfile support - now version upgrades happens only when the user chooses to do so.

Usability

I've been using paket now for a few days, and it's pretty easy to use, with good documentation. Once we have bazel support as well, I'll provide a script to run when introducing/updating new dependencies that'll update the lockfile and the bazel interpretation of that.

[michaelnebel]

Very good work and thorough explanation and description! Thank you!

This looks plausible to me, but I would also like one more person from the C# team to take a look.

In the meantime here are some questions :-)

• What is the workflow for generating paket.lock and Paket.Restore.targets.
• Today we use a VS code plugin for updating NuGet packages (NuGet Package Manager GUI). Is there any tool that you can recommend for updating paket.dependencies?
• Could you be persuaded to update our documentation for the “workflow” for updating the dependencies (basically the questions above): https://github.com/github/codeql-csharp-team/blob/master/UpdatingRoslyn.md.

[criemen]

Hi, thanks for the review!

I agree we should also get a second set of eyes on this.

What is the workflow for generating paket.lock and Paket.Restore.targets.

dotnet paket install updates these files after one added a new dependency to paket.dependencies, and to the individual paket.references. If you wawnt to update all transitive dependencies, dotnet paket update will do the job. Note that we're following the nuget minimum version selection algorithm, so you also need to manually bump the version numbers in paket.dependencies.
We could switch to pulling in the maximum version number, and not pin to specific numbers in paket.dependencies, but rather drop the version constraints or relax them (to pin only major versions of the relevant C# analysis packages?), to make the dependency update mechanism easier.

Today we use a VS code plugin for updating NuGet packages (NuGet Package Manager GUI). Is there any tool that you can recommend for updating paket.dependencies?

See above - I think by relaxing version requirements, we'd reduce the effort to update dependencies to dotnet paket update. Otherwise, I don't know of any tools for managing the external dependencies.

Could you be persuaded to update our documentation for the “workflow” for updating the dependencies (basically the questions above): https://github.com/github/codeql-csharp-team/blob/master/UpdatingRoslyn.md.

As converting the build to bazel introduces an additional step in updating the dependencies (the paket lockfile needs to be converted in a separate step to a bazel file), I'll provide a script to execute all the necessary commands in one go.
Would you mind if I file a PR for that document once everything for bazel is in place, which should happen in 1-2 weeks?

[michaelnebel]

• It would prefer to pin Roslyn to a specific version and only update/change that on purpose (There might be instabilities or small semantic differences).
• Yes, it it perfectly fine to wait with the documentation. We just need something for the next C# update :-)

[criemen]

Pinning Roslyn more makes a lot of sense! I'd like to merge this PR as-is, and then we can improve the dependency version constraints as follow-up, so that the internal tests on the external PRs correctly run - right now, they need the internal PR to be merged that's waiting on merging this PR.

[criemen]

Internal CI now passes on this PR :)

[tamasvajk]

Some comments

Deduplication of version numbers

I think this is not a real advantage over sln/csproj files. We can achieve the same with csproj files too by adding a top level Directory.Packages.props file.

Reproducible builds with a lockfile

This can be achieved with nuget too. See for example here.

I believe the main drawback is having to run dotnet tool restore before dotnet restore/dotnet build will work.

I think we should be able to work around this limitation by adding a Directory.Build.targets file into the root folder, which automatically runs the command before some appropriate targets. The below doesn't work, but might help getting to a solution:

<Project>
  <Target Name="LocalToolRestore" BeforeTargets="PaketRestore">
    <Message Text="Restoring tools" Importance="High" />
    <Exec Command="dotnet tool restore" />
  </Target>
</Project>

Aren't there other drawbacks? For example,

• we won't be able to analyse our own codebase with buildless extraction. :-(
• Theql/csharp folder is not going to be a self contained C# folder any longer, because of the top level .paket folder and paket.* files. :-(

First experience with paket

I've tried the following:

~/dev/semmle-code/ql/csharp
❯ gh pr checkout 16376
...
❯ dotnet tool restore
...
❯ dotnet build
...

This results in .paket/Paket.Restore.targets showing up as changed:

warning: in the working copy of '.paket/Paket.Restore.targets', CRLF will be replaced by LF the next time Git touches it

How should I configure my git so that this doesn't happen?

The above dotnet build shows a lot of seemingly non-C# extractor related build warnings. Here are a couple these:

  Unable to parse project /Users/tamasvajk/dev/semmle-code/ql/go/ql/test/.project:
        unable to find Project node in file /Users/tamasvajk/dev/semmle-code/ql/go/ql/test/.project
  Unable to parse project /Users/tamasvajk/dev/semmle-code/ql/python/ql/test/.project:
        unable to find Project node in file /Users/tamasvajk/dev/semmle-code/ql/python/ql/test/.project
  Restoring /Users/tamasvajk/dev/semmle-code/ql/csharp/ql/test/resources/stubs/System.Reflection/4.3.0/System.Reflection.csproj
  Restoring /Users/tamasvajk/dev/semmle-code/ql/csharp/ql/integration-tests/posix-only/standalone_dependencies_multi_project/standalone1.csproj

It looks like we try to parse/restore all .project, .csproj files in the ql directory, but I issued the build command in ql/csharp, so my expectation would be that we only restore projects in that folder. Also, I think we should be more specific about which .csproj files are restored. We don't have to restore projects in ql/csharp/ql/test/resources/stubs or ql/csharp/ql/integration-tests. I think, we only need to restore the ones that are (transitively) referenced in ql/csharp/CSharp.sln. Can we configure paket to limit the files that it tries to restore?

Second experience with paket

If I rerun dotnet build, then these unexpected restores don't happen. If I delete ql/paket-files/paket.restore.cached, the unexpected restores do happen. Do you happen to know where paket keeps its cache? (paket.restore.cached only contains hash values.)

Also, after deleting ql/paket-files/paket.restore.cached and running dotnet build, I'm getting the below:

/Users/tamasvajk/dev/semmle-code/ql/.paket/Paket.Restore.targets(171,3): error MSB3073: The command "dotnet paket restore" exited with code 1. [/Users/tamasvajk/dev/semmle-code/ql/csharp/extractor/Semmle.Extraction.Tests/Semmle.Extraction.Tests.csproj]
    0 Warning(s)
    1 Error(s)

Other times I'm getting more errors:

/Users/tamasvajk/dev/semmle-code/ql/.paket/Paket.Restore.targets(171,3): error MSB3073: The command "dotnet paket restore" exited with code 1. [/Users/tamasvajk/dev/semmle-code/ql/cpp/autobuilder/Semmle.Autobuild.Cpp/Semmle.Autobuild.Cpp.csproj]
/Users/tamasvajk/dev/semmle-code/ql/.paket/Paket.Restore.targets(171,3): error MSB3073: The command "dotnet paket restore" exited with code 1. [/Users/tamasvajk/dev/semmle-code/ql/csharp/extractor/Semmle.Util/Semmle.Util.csproj]
    0 Warning(s)
    2 Error(s)

And sometimes the build succeeds.

IDE integration

I'm using VS Code with C# Dev Kit. It is working as expected:

Starting Restore solution...
Completed Open a solution (68302ms)
Starting NuGet restore for the solution.
Starting command: "dotnet" restore /Users/tamasvajk/dev/semmle-code/ql/csharp/CSharp.sln --interactive...
Completed command: "dotnet" restore /Users/tamasvajk/dev/semmle-code/ql/csharp/CSharp.sln --interactive (71574ms)
Completed NuGet restore.
Completed Restore solution (71584ms)

Note that it relies on ql/csharp/CSharp.sln.

Others on the team might be using other IDEs, it would be worth checking if basic navigational features work there too.

Overall, paket seems to be working. I need to learn a bit more about it to understand what happens under the hood, especially, where some of the caches are stored and how it chooses initially the files to parse/restore, but I don't see anything major that would be blocking this change.

[criemen]

Thanks Tamas!

I'll address your questions in multiple rounds, as I need to do some research first.

Re deduplication of version numbers/lockfile: Yes, I'm not claiming this is the only way to achieve this, but it's part of the PR. Unfortunately, right now this is the only way to achieve those goals while also supporting a bazel build.

Re Directory.Build.targets: I'll investigate that, thanks for the pointer!

The ql/csharp folder is not going to be a self contained C# folder any longer

Yes I'm sad about that, too. I don't really know what to do. This is maybe connected with the restore command trying to restore all .project files recursively - if I don't find a way to stop paket from doing that, I'll have to move the paket installation to ql/csharp anyways. Then we're either left with two separate paket installations for C# and the cpp win autobuilder (not so great, as they share the utility packages!), or we move the cpp win autobuilder into ql/csharp and leave a note in ql/cpp/autobuilder. Now that I'm thinking about it, maybe that's the best solution after all (not a great one, though).

How should I configure my git so that this doesn't happen?

I'll try to find the correct incantation in .gitattributes that you don't get that error.

The above dotnet build shows a lot of seemingly non-C# extractor related build warnings

see above, also: https://fsprojects.github.io/Paket/paket-folder.html says

When Paket encounters paket.dependencies files in subdirectories it ignores that subdirectory (and everything under it) entirely, implying that they use an independent paket.lock file and packages directory. The packages directory will be created at the root level for all projects under it.

which means we can ignore sub-folders, but (unless there's another feature I've not found) only by creating a file in them. Imo that's acceptable for sub-folders inside ql/csharp, but not under other subfolders of ql. That's another argument towards moving the paket installation outside of the top-level folder.

Can we configure paket to limit the files that it tries to restore?

Either we manage to pass more specific command line options to paket through the implicit invocations of the paket CLI through dotnet build (which might be tricky, as the build targets for that are auto-generated, or we block those folders by placing paket.dependencies files inside ql/csharp/ql and integration test folders.

Do you happen to know where paket keeps its cache?

https://fsprojects.github.io/Paket/caches.html#NuGet-machine-cache mentions using the nuget machine cache.

Also, after deleting ql/paket-files/paket.restore.cached and running dotnet build, I'm getting the below:

I'll investigate. Getting different errors looks like a concurrency thing to me.

Others on the team might be using other IDEs, it would be worth checking if basic navigational features work there too.

Which would those be? I was hoping that Visual Studio would be working, if the IDE integration for VSCode picks up the dependencies.

Apropos IDE: There's some IDE support for paket files and commands available here: https://fsprojects.github.io/Paket/editor-support.html.

[tamasvajk]

Which would those be? I was hoping that Visual Studio would be working, if the IDE integration for VSCode picks up the dependencies.

We're not using the full blown VS, because we're all on MacOS. So I think Rider, and the discontinued VS for Mac are the two other IDEs that might be relevant. cc @michaelnebel @hvitved which IDE are you using?

[tamasvajk]

@criemen In the meantime I have an answer too:

The paket.restore.cached file is read by the .paket/Paket.Restore.targets. And it is actually not pointing to any other cache than the paket.lock. shasum -a 256 paket.lock returns the same SHA256 hash that's stored in the paket.restore.cached file.

[criemen]

I filed #16487 to move the C++ Windows autobuilder into ql/csharp. I'll try to make more progress on the rest of the comments here, but as the entire C++ team is at an offsite, I don't expect much (visible) progress unfortunately.

[jketema]

I filed #16487 to move the C++ Windows autobuilder into ql/csharp. I'll try to make more progress on the rest of the comments here, but as the entire C++ team is at an offsite, I don't expect much (visible) progress unfortunately.

Do you want explicit feedback from the C/C++ team? Most recent changes have been driven by the C# team, and we've generally assumed that they know what they were doing, not requiring explicit approvals from the C/C++ team. I'd assume the same thing here?

[criemen]

@jketema I'd like explicit approval from the C++ team (and C# too) on #16487, and for everything else I'll rely on the C# team and Paolo for the bazel bits (coming in the future).
I know you're on your meetup, so if it's a quick approval of the methodology that'd be great, but otherwise I'll wait for you to be back next week.

[michaelnebel]

Which would those be? I was hoping that Visual Studio would be working, if the IDE integration for VSCode picks up the dependencies.

We're not using the full blown VS, because we're all on MacOS. So I think Rider, and the discontinued VS for Mac are the two other IDEs that might be relevant. cc @michaelnebel @hvitved which IDE are you using?

I use VS Code with C# Dev kit.

[criemen]

I've worked some more on this, now that the C++ autobuilder has been moved.

I think we should be able to work around this limitation by adding a Directory.Build.targets file into the root folder, which automatically runs the command before some appropriate targets. The below doesn't work, but might help getting to a solution:

I got this working in so far as dotnet tool restore is being executed. It doesn't work, though, as it executes the restore command 8 times or so in parallel, and dotnet restore isn't safe to be executed in parallel - it stomps over itself when writing the package files to disk. I was unable to find a msbuild solution that runs a Task one after each other.
Do also note that we (for that reason!) build all our C# projects in a chain, instead of in parallel.
Therefore, I don't think running dotnet tool restore automatically via msbuild is feasible, but I don't know msbuild very well - if there's a trick to execute the task only once, that'd work.

[tamasvajk]

Therefore, I don't think running dotnet tool restore automatically via msbuild is feasible, but I don't know msbuild very well - if there's a trick to execute the task only once, that'd work.

I haven't checked this, but I think we could added a before.CSharp.sln.targets file, which could contain targets that only need to be run once. See here. This file would not be used by sembuild or any of our tooling (which ignore the .sln file), but could be useful for contributors or us when we're working only in the IDE.

[criemen]

I'll check that out!

I also played with deleting the paket cache file, and I couldn't reproduce the paket errors you reported above.
Internal CI on this PR will pass by EOD.

Besides playing with the before target for local development, is there anything I've missed from your comments Tamas? How are you feeling about the state of the PR now?

[tamasvajk]

Besides playing with the before target for local development, is there anything I've missed from your comments Tamas? How are you feeling about the state of the PR now?

Let's check this small issue, and then I think the PR can be merged.

[criemen]

Great, I made it work with the before file, good find! This only works for CSharp.sln, but that should improve the DX for people building that localy, and for CI we specify dotnet tool restore manually.
The internal CI also ought to pass now on this PR, so if that's indeed the case, this PR could go in from my POV.

[tamasvajk]

The changes look reasonable to me. Have you checked the performance of the build before and after this change?

I'm seeing very high CPU usage during restores, so high that my VSCode and other windows keeps freezing. At the same time, there's significant slowdown in the build locally.

Running dotnet build once to do the restore, and then measuring the exec time of a second run results in the below. (Only the first one should be doing a restore, the latter one should just build.)

On main:

❯ time dotnet build /t:rebuild
  Determining projects to restore...
  All projects are up-to-date for restore.
...
Time Elapsed 00:00:17.10
dotnet build /t:rebuild  8.69s user 1.77s system 59% cpu 17.717 total

On criemen/dotnet-paket:

❯ time dotnet build /t:rebuild
...

Time Elapsed 00:01:17.71
dotnet build /t:rebuild  13.14s user 17.39s system 38% cpu 1:18.43 total

It's quite a big change. Also, I think the only difference should be the package restore, but the run with paket also lists vulnerable package messages to the console, such as warning NU1903: Package 'System.Text.RegularExpressions' 4.3.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-cmhx-cq75-c4mj, so it's definitely doing more work than the restore with nuget.

[criemen]

Can you try once more with one more repetition of the rebuild? What machine do you have?
High CPU usage might be also Defender scanning 😬

On my local machine (Mac M1) I get the following three timings (in sec),
for running dotnet build /t:rebuild in ql/csharp:
main: 12/3/3
paket branch: 27/7/6

Nuking all caches I could find (including git clean -fxd in ql/csharp, and removing the nuget cache on my system), I get:
paket branch

1. time dotnet build . 51sec (a second run of this in clean conditions yielded 32sec, so there's quite some timing instability in this)
2. time dotnet build . /t:rebuild: 10sec
3. time dotnet build . /t:rebuild: 6sec

main:

1. time dotnet build . 20sec
2. time dotnet build . /t:rebuild: 3sec
3. time dotnet build . /t:rebuild: 3sec

So indeed this is slower, but not egregiously so for repeated builds.
Removing before.CSharp.sln.targets would speed up builds further by ~1sec, more for the clean case (but that's when you'd need to run dotnet tools restore by hand).

[tamasvajk]

I think the high CPU usage is caused by paket. There are multiple dotnet processes with high CPU utilization.

I'm getting the below on the paket branch:

Time Elapsed 00:00:51.56
dotnet build /t:rebuild  9.39s user 12.06s system 41% cpu 51.992 total

And on main:

Time Elapsed 00:00:07.39
dotnet build /t:rebuild  3.11s user 0.89s system 51% cpu 7.757 total

I'm on an intel based mac (2.4 GHz 8-Core Intel Core i9, 64GB memory).

[tamasvajk]

Let's merge this, and then we'll figure out if we can improve the performance.

Running with diagnostics logs (dotnet build /t:rebuild /v:diag), the below targets are the ones that take longer than 1s:

     1002 ms  LocalToolRestore                           2 calls
     1273 ms  GenerateBuildDependencyFile               16 calls
     1820 ms  CoreClean                                 16 calls
     2793 ms  CoreCompile                               16 calls
     5398 ms  _CreateAppHost                             6 calls
     6685 ms  CleanReferencedProjects                   16 calls
     7735 ms  _GetProjectReferenceTargetFrameworkProperties  16 calls
     7746 ms  _GenerateRestoreGraph                      1 calls
    14759 ms  _GitRepositoryUrl                         10 calls
    16429 ms  _GitBranch                                10 calls
    16686 ms  _GitCommitDate                            10 calls
    17930 ms  _GitBaseVersionTagExists                  10 calls
    18609 ms  _EnsureGit                                10 calls
    26560 ms  Rebuild                                   17 calls
    33449 ms  _GitCommit                                10 calls
    44216 ms  _GitBaseVersionTag                        10 calls
    80897 ms  _GitRoot                                  10 calls
    119636 ms  PaketRestore                              32 calls

I don't yet understand why we spend this much time in PaketRestore on an already restored solution. But I couldn't quickly figure this out from the logs. There are other performance optimizations that we should look into, such as is it worth calling all the _Git* targets 10 times? Are those calls always returning the same data? Why are we calling LocalToolRestore twice? ...

[criemen]

Why are we calling LocalToolRestore twice? ...

I tried to fix that by 74e446e, but that didn't work. Something in dotnet build runs the InitialTargets set twice. Using BeforeTargets="PaketRestore" didn't work for me, maybe because at that time PaketRestore isn't defined yet? I'm no MSBuild expert, though.
It'll be interesting to see if we run the _Git* targets at all once we move from the GitInfo package to having bazel supply the git information.

I'm surprised that paket restore is invoked more than once per project - we have 17 projects here, but 32 (so almost 2x) paket invocations.

Thanks for the approval, I also have a follow-up that gets rid of some transitive dependencies, hopefully that'll speed up the paket restore experience for you!