Skip to content
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

NuGet updates #105

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

NuGet updates #105

wants to merge 4 commits into from

Conversation

bording
Copy link
Member

@bording bording commented Oct 23, 2024

This PR makes changes to prepare for handling vulnerabilities reported by NuGet's security audit feature.

  • We're now explicitly setting NuGetAuditMode in Directory.Build.props to all to match the default value coming in the 9.0.100 SDK. This value can be set in Custom.Build.props to override the value globally, something we might end up needing to do on older release branches, for example.
  • To make it easier to update PackageReference versions and to eliminate the need for duplicate references in test projects, the automatic version ranges feature added here will automatically create the version ranges for any packages that are created.
  • Dependabot stopped considering the Feedz feed for its package updating portion of its process, so packages that are only on Feedz weren't being updated. This seems like a bug, but adding it to the dependebot.yml makes it work again, so for now we'll stick with that.

Comment on lines +3 to +8
<PropertyGroup>
<AutomaticVersionRangesEnabled Condition="'$(AutomaticVersionRangesEnabled)' == '' And '$(Configuration)' == 'Debug'">false</AutomaticVersionRangesEnabled>
<AutomaticVersionRangesEnabled Condition="'$(AutomaticVersionRangesEnabled)' == '' And '$(IsPackable)' == 'false'">false</AutomaticVersionRangesEnabled>
<AutomaticVersionRangesEnabled Condition="'$(AutomaticVersionRangesEnabled)' == '' And '$(ManagePackageVersionsCentrally)' == 'true'">false</AutomaticVersionRangesEnabled>
<AutomaticVersionRangesEnabled Condition="'$(AutomaticVersionRangesEnabled)' == ''">true</AutomaticVersionRangesEnabled>
</PropertyGroup>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: As I understand it, the new setting will be enabled by default. How we will disable it?
It will be enough to add <AutomaticVersionRangesEnabled>true</AutomaticVersionRangesEnabled> to the Custom.Build.props file on the repo?

Copy link
Member Author

@bording bording Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be set to false, not true, but otherwise, yes. It can be set in Custom.Build.props to disable it globally, or in an individual project as needed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry about that!

<AutomaticVersionRangesEnabled Condition="'$(AutomaticVersionRangesEnabled)' == ''">true</AutomaticVersionRangesEnabled>
</PropertyGroup>

<UsingTask TaskName="ConvertToVersionRange" TaskFactory="RoslynCodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll" >
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing is wonky on this block, 1 on the start element, 2 on the end, no indentation for Task. Is there a reason to have the rest of the file left-aligned despite the wrapping Project element?


<UsingTask TaskName="ConvertToVersionRange" TaskFactory="RoslynCodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll" >
<Task>
<Code Source="$(MSBuildThisFileDirectory)ConvertToVersionRange.cs" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider finding a name for a subfolder to put this file and the C# file in. The more I think about it the more I think people are going to be very confused seeing a C# file in the src folder, and may try to move/remove it, leading to resync PRs. I know targets is a poor choice because of https://github.com/Particular/NServiceBus.RabbitMQ/tree/master/src/targets but what about packaging or packaging-targets? Long term the strong name keys could also move into such a folder to have them more out of the way, but that's obviously a much bigger change that would also require cleanup. Something to think about, anyway.

</Task>
</UsingTask>

<Target Name="ConvertProjectReferenceVersionsToVersionRanges" AfterTargets="_GetProjectReferenceVersions" Condition="'$(AutomaticVersionRangesEnabled)' == 'true'">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's touching private targets, have you tried this with the net9 SDK, since we're only a few weeks from that?

</Task>
</UsingTask>

<Target Name="ConvertProjectReferenceVersionsToVersionRanges" AfterTargets="_GetProjectReferenceVersions" Condition="'$(AutomaticVersionRangesEnabled)' == 'true'">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just curious, what is an example of a project reference with a version?

continue;
}

var privateAssets = reference.GetMetadata("PrivateAssets");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works regardless of if it's as an XML attribute or sub-element, correct?

}

var version = reference.GetMetadata(VersionProperty);
var match = Regex.Match(version, @"^\d+");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how the code is being included in MSBuild, can it use the Regex source generators?


var nextMajor = Convert.ToInt32(match.Value) + 1;

var versionRange = $"[{version}, {nextMajor}.0.0)";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't have to type all these anymore, is there a way to make a range that accounts for the brokenness of NuGet version ranges with respect to prereleases of the next major? I didn't re-read that thread again, but would [7.0.0, 8.0.0-0) work and is it worth it to try doing that? Or would that just result in a bunch of weird packages that would be super-odd if Microsoft ever decides to fix NuGet/Home#6434 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants