-
Notifications
You must be signed in to change notification settings - Fork 40
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
Implement version pinning from PackageLineup's #362
Conversation
@natemcmaster How does the tooling deal with this? What happens when you look at the package manager UI with these packages? Does it work well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just clarifications, looks fine overall.
/// <summary> | ||
/// e.g. "Configuration=Debug;BuildNumber=1234" | ||
/// </summary> | ||
public string Properties { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that these properties could be different depending on the policies we try to enforce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Properties' is a little vague. I'm going to rename to 'ProjectProperties'. This are properties meant to be applied to projects and solutions when they are loaded. This task executes a design-time build in order to examine target frameworks, package references, and a few other details.
/// Applies policies to a repository on how NuGet can be used, such as restricting which PackageReference's are allowed, | ||
/// which versions of packages can be used, which feeds are available, etc. | ||
/// </summary> | ||
public class ApplyNuGetPolicies : Microsoft.Build.Utilities.Task, ICancelableTask |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want this to be an ICancelableTask
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Ctrl+C doesn't orphan a potentially long-running network request.
/// <summary> | ||
/// TaskLoggingHelper -> ILogger | ||
/// </summary> | ||
internal class MSBuildLogger : LoggerBase, Common.ILogger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we already have this somewhere else or do we imagine this will be the only place that needs it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implements NuGet's ILogger
to forward to MSBuild. At the moment, I don't think we use any other NuGet APIs that require ILogger.
<PackageReference Include="Newtonsoft.Json" Version="$(JsonNetInMSBuildVersion)" PrivateAssets="All" /> | ||
</ItemGroup> | ||
|
||
</Project> | ||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline.
modules/NuGet.Tasks/module.targets
Outdated
</PropertyGroup> | ||
|
||
<ItemGroup Condition="@(PackageLineup->Count()) != 0 AND '$(RestrictVersionOnPackageReference)' != 'false' "> | ||
<!-- Automatically warn about package reference with versions this when lineups are used. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"with versions this when..."? Not sure what you're getting at here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grammar error. it said something different at first and i failed to delete "this"
PackageLineup can be specified in repo.props/targets as a way to pin a set of package references. This adds support to KoreBuild for using a nuget package as a lineup, and to pin the versions.
PackageLineup can be specified in repo.props/targets as a way to pin a set of package references. This adds support to KoreBuild for using a nuget package as a lineup, and to pin the versions.
Details: https://github.com/aspnet/BuildTools/wiki/%5Bspec%5D-Package-version-lineups
Part of aspnet/KoreBuild#239.
Example usage:
To use this feature, our repos can make the following changes:
Version
attribute from PackageReferencecc @mikeharder @davidfowl