-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Update harvestPackages.depproj to latest stable #30172
Conversation
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.
LGTM.
Cc: @weshaggard
@@ -2,19 +2,19 @@ | |||
<!-- The versions can be updated by running UpdateToLatestStablePackages target in harvestPackages.depproj --> | |||
<ItemGroup> | |||
<PackageReference Include="Microsoft.CSharp"> <Version>4.5.0</Version></PackageReference> | |||
<PackageReference Include="Microsoft.VisualBasic"> <Version>10.2.0</Version></PackageReference> | |||
<PackageReference Include="Microsoft.VisualBasic"> <Version>10.3.0</Version></PackageReference> |
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.
What are all of these ^M instances?
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.
Those are added by the target that automatically updates this file.
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 guess something to do with the text format when writing to the file.
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.
Looks like they were present before as well: https://github.com/dotnet/corefx/blob/release/2.1/external/harvestPackages/harvestPackages.props
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.
It'd be nice if we could fix the tool to not do that :)
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.
Seems like it's coming from here:
<_Lines Include="<PackageReference Include="%(_NewPackageReferences.Identity)"> <Version>%(_NewPackageReferences.Version)</Version> </PackageReference>" /> |
and specifically that
.
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.
@stephentoub let's do that in a separate PR, @mmitche needs this in ASAP so he can spin a servicing build for 2.1
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.
Yup, totally fine. It just jumped out at me (in large part because GitHub highlights all of them in red ;)
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.
Yes agreed we should fix that formatting, that was me putting the wrong encoding a long time ago.
@mmitche feel free to queue the 2.1 build once this gets through the mirror |
This is the result of running
/t:UpdateToLatestStablePackages
onharvestPackages.depproj
, plus addingSystem.Net.WebSockets.WebSocketProtocol
.CC @mmitche @safern