Add parsing of MSBuild SDK dependencies (NuGet)

[Zastai]

This adds the three ways to specify an MSBuild SDK reference inside a project file, including test cases for each (parsing and updating).

Intended to fix #2839

Note: I'm new to Ruby, but do have access to the JetBrainz IDEs, which has helped. So please forgive novice code issues.

[Zastai]

Hmm - looks like I missed that while the actual tests pass, the coverage is complaining about a 0.02% drop.
Will see what I can do about that.

[Zastai]

Not a lot, it seems. Well, I could remove two existing-but-unused methods (declaration_nodes in the two finders), but that has nothing to do with what this PR is about, so seems like the wrong way to go.

[Zastai]

Would be nice to get some feedback on this...

[jurre]

Thanks for doing this work @Zastai, I'll try to make some time next week to thoroughly review this and get a bit more familiar with MSBuild SDK references. Just wanted to let you know that we appreciate the contribution.

[japj]

Hello, great to see this (I recently “discovered” dependabot).
We have been building our own MSBuild Sdk for a while now and we actually use a different way to specify the sdk version (that is documented in the link you provided https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-use-project-sdk)

The global.json can have an msbuild-sdks section that can be modified to “upgrade” the sdk for all csproj files that use that sdk:

{
    "msbuild-sdks": {
        "My.Custom.Sdk": "5.0.0",
        "My.Other.Sdk": "1.0.0-beta"
    }
}

so in that case, just modifying the global.json is enough.
Could you perhaps add that behavior to this PR?

[japj]

Sorry, I just read #2839 where you mention that global.json support is already in?

[Zastai]

That was the plan - but then I found that global.json already had that behaviour:

dependabot-core/nuget/lib/dependabot/nuget/file_parser/global_json_parser.rb

Line 23 in 3bca853

project_sdks = parsed_global_json.fetch("msbuild-sdks", {})

[Zastai]

@jurre Any update?

[Zastai]

@jurre
It has now been 8 months since I created this PR. Would be nice to get this properly reviewed.

[Zastai]

We're rapidly approaching this PR's one year anniversary...
Any chance of a review, @jurre?

[jurre]

Overall these changes look good, I have made some mostly superficial suggestions regarding readability and rubyisms.

I am not super familiar with how the newly added functionality is typically used in NuGet so I'm having somewhat of a hard time reviewing for correctness.

I think @xlgmokha has some more experience on this so I would love to get his eyes on this as well.

Do you happen to have a sample project I could test this end-to-end on?

Apologies for the slow response on this on my part, it definitely fell through the cracks a couple of times and going to try to land this ASAP.

[jurre]

Suggested change

	declaring_file.content.scan(regex).each do |string|
	xml = string
	declaring_file.content.scan(regex).each do |xml|

[Zastai]

Applied

[Zastai]

Actually, I'm having to revert this - string needs to be used unchanged afterwards, while xml potentially gets modified.

[jurre]

fwiw string will also get modified:

irb(main):004:0> string = "foo"
irb(main):005:0> xml = string
irb(main):006:0> xml << "x"
=> "foox"
irb(main):007:0> string
=> "foox"

[Zastai]

It would if I used <<, but I use xml += and it's my understanding that that creates a new string.

[jurre]

Ah that's correct, but looking at the code we never refer to string after xml is changed, at least not now that the code is extracted to parse_element. Anyway, it's not really important I guess

[Zastai]

We do in sdk_strings: strings << string; that's why parse_element() retains the +=.

[Zastai]

I should be able to make a recipe for setting up an otherwise-empty project with each form of reference, if that suits.
All it would need is an installed .NET SDK (or .NET Core SDK).

[Zastai]

I think I addressed all your remarks, @jurre.

Sample projects are present as fixtures, but in order to set up a repo for "real" testing, here are the recipes.

Recipes

Each recipe starts from an empty directory and sets up a dependency on an old version of an SDK package I maintain (MetaBrainz.Build.Sdk).
Dependabot should suggest an update for it.

A. Using global.json (already supported by dependabot)

1. Create a .csproj file (name can be chosen freely) containing

   <Project Sdk="MetaBrainz.Build.Sdk">
   </Project>
   

2. Create a global.json file containing

   {
     "msbuild-sdks": {
       "MetaBrainz.Build.Sdk" : "1.0.0"
     }
   }
   

B. Using a version specifier in the Project tag (1)

1. Create a .csproj file (name can be chosen freely) containing

   <Project Sdk="MetaBrainz.Build.Sdk/1.0.0">
   </Project>
   

C. Using a version specifier in the Project tag (2)

This is the same case as B, but it checks for support for multiple SDK specifications.

1. Create a .csproj file (name can be chosen freely) containing

   <Project Sdk="Microsoft.NET.Sdk;MetaBrainz.Build.Sdk/1.0.0">
   </Project>
   

D. Using an Sdk tag

1. Create a .csproj file (name can be chosen freely) containing

   <Project>
   
     <Sdk Name="MetaBrainz.Build.Sdk" Version="1.0.0" />
   
   </Project>
   

E. Using an Sdk attribute on an Import tag

1. Create a .csproj file (name can be chosen freely) containing

   <Project>
   
     <Import Sdk="MetaBrainz.Build.Sdk" Project="Defaults.props" Version="1.0.0" />
     <Import Project="CSharp.props" Version="1.0.0" Sdk="MetaBrainz.Build.Sdk" />
   
     <Target Name="Dummy" />
   
   </Project>
   

[Zastai]

Bit confused that tests are failing on a part of Dependabot not touched by this PR (it tries to use pandoc which is not installed).
Is CI broken on main/master or something?

[jurre]

Bit confused that tests are failing on a part of Dependabot not touched by this PR (it tries to use pandoc which is not installed). Is CI broken on main/master or something?

I don't see this failures you're referring to (maybe they were fixed after a retry). I do see a few linting errors for nuget that we'll need to address, and there are a few test suites that are unfortunately sometimes flaky, if a suite other than nuget fails in this PR I can retry/fix it on main though.

[Zastai]

That's strange; I don't see it now either - only a style check. I'll push an update for that to see if that fixes the checks.

[Zastai]

Ah I see it now. I updated my fork, and it looks like that updated the CI workflow to include a scheduled run, which triggered and failed. Looks like I had best disable Actions on my fork.

[xlgmokha]

I think @xlgmokha has some more experience on this so I would love to get his eyes on this as well.

LGTM. I'm not sure where/why pandoc is used but I'll take a peek. Otherwise...

[xlgmokha]

It looks like a new version of pandoc-ruby was released recently that includes a change in behaviour that is triggering a failure in the test suite. This repo doesn't seem to include a version controlled Gemfile.lock so the resolved version changed and caused this failure. I'll try to get this fixed up in the main branch so that we can get these changes released.

[xlgmokha]

I'm working on a fix in #4412 that should restore the common build to a healthier state. Unfortunately, that means we'll need to update this branch which is something I don't have access to do.
@Zastai Do you mind pulling in the latest when #4412 is merged? I'll take care of the rest.

[Zastai]

Yeah, no worries - ping me here when it lands and I'll pull in main.

[Zastai]

@xlgmokha Main merged; hopefully that will allow all the checks to pass.

[xlgmokha]

question: could this be abused by an XML injection attack?

[Zastai]

I do not know. It has been a long time since I created the PR, but I think I copied what was already there for other processing (like PackageReference).

[xlgmokha]

Thank you for your contribution @Zastai.