Apply source-build patches #4404

crummel · 2022-01-13T18:58:02Z

Bug

Fixes: dotnet/source-build#2708

Regression? No

Description

This PR moves the source-build patches from 6.0 back to NuGet. The patches are:

Re-enabling full-framework builds in source-built NuGet. Source-build now has better support for full-fx and this allows other repos to depend on NuGet's full-fx build.
Upgrade CommandLineUtils to CommandLineUtils.Sources. This aligns with getting all repos off the ancient CommandLineUtils version and should not be a functional change.
Update #ifdefs to include .NET 6 as well as .NET 5 since source-build 6.0 must build with the 6.0 SDK.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- OR
  There should be no changed behavior in these patches in the NuGet repo. I will test this in the source-build build specifically.
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled
- OR
- N/A

nkolev92

Is the SDK going to run under net6.0?

I thought the plan was net5.0, then net7.0 and skipping net6.0.

src/NuGet.Core/NuGet.CommandLine.XPlat/NuGet.CommandLine.XPlat.csproj

src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRequestMessageExtensions.cs

build/packages.targets

crummel · 2022-02-01T15:52:11Z

Unfortunately we won't be able to skip net6 because Microsoft.Build 17.0.0 only supports net472 and net6. I logged dotnet/source-build#2745 to track these TFM changes and back them out once we can move to net7.

zivkan · 2022-02-01T16:57:24Z

src/NuGet.Core/NuGet.Common/NuGet.Common.csproj

+  <ItemGroup Condition=" '$(TargetFramework)' == '$(NETFXTargetFramework)' or '$(TargetFramework)' == 'net45' or '$(TargetFramework)' == 'netstandard2.0' ">
    <Reference Include="System.Core" />
    <Reference Include="System.IO.Compression" />
    <Reference Include="mscorlib" />


Doesn't the .NET SDK automatically add these BCL assembly references for .NET Standard projects? My gut feeling is that adding the netstandard2.0 condition to the item group should be unnecessary.

build/common.project.props

zivkan · 2022-02-01T17:10:02Z

src/NuGet.Core/NuGet.Configuration/NuGet.Configuration.csproj

+  <ItemGroup Condition=" '$(TargetFramework)' == '$(NETFXTargetFramework)' or '$(TargetFramework)' == 'net45' or '$(TargetFramework)' == 'netstandard2.0' ">
    <Reference Include="System.Security" />
    <Reference Include="System.Xml" />
    <Reference Include="System.Xml.Linq" />


Same as previous comment, doesn't the .NET SDK add these BCL references automatically for .NET Standard?

The netstandard condition was removed from NuGet.Common.csproj, but not here.

zivkan · 2022-02-01T17:13:21Z

src/NuGet.Core/NuGet.Configuration/NuGet.Configuration.csproj

    <TargetFrameworks>$(TargetFrameworksLibrary)</TargetFrameworks>
    <TargetFrameworks Condition="'$(IsBuildOnlyXPLATProjects)' != 'true'">$(TargetFrameworks);net45</TargetFrameworks>
+    <TargetFrameworks Condition="'$(IsBuildOnlyXPLATProjects)' == 'true'">$(TargetFrameworks);netstandard2.0</TargetFrameworks>


if my suggestion in common.project.props can't be taken, given that the last two of these are opposites of each other, can we get rid of the first, non-conditioned, declaration, so MSBuild doesn't have to overwrite or re-evaulate the same property multiple times in all scenarios? I'm not enough of an MSBuild expert to know whether or not it has an actual performance benefit, but maybe it's clearer to readers to me because I will need to do less mental arithmetic to figure out which set declarations contribute to the final value.

zivkan · 2022-02-01T17:15:30Z

src/NuGet.Core/NuGet.Common/NuGet.Common.csproj

@@ -6,6 +6,7 @@
    <Description>Common utilities and interfaces for all NuGet libraries.</Description>
    <TargetFrameworks>$(TargetFrameworksLibrary)</TargetFrameworks>
    <TargetFrameworks Condition="'$(IsBuildOnlyXPLATProjects)' != 'true'">$(TargetFrameworks);net45</TargetFrameworks>
+    <TargetFrameworks Condition="'$(IsBuildOnlyXPLATProjects)' == 'true'">$(TargetFrameworks);netstandard2.0</TargetFrameworks>


off topic for this PR, but given that common.project.props defines <IsBuildOnlyXPLATProjects>$(DotNetBuildFromSource)</IsBuildOnlyXPLATProjects>, should we use DotNetBuildFromSource instead?

I'm the team's CI "expert", but it's not clear to me what is source-build specific, and what we do in our own CI differently on Windows vs Mac/Linux. I think if we removed IsBuildOnlyXPLATProjects and used DotNetBuildFromSource instead throughout our build scripts, it would be much easier to understand what is what.

dominoFire · 2022-02-01T18:57:35Z

@crummel You might need to rebase your branch. CI build 6.1.0.757 has an outdated build number. Current build numbers starts with 6.2.

src/NuGet.Core/NuGet.Configuration/NuGet.Configuration.csproj

zivkan · 2022-02-02T21:25:08Z

The CI build is failing because Microsoft.Extensions.CommandLineUtis.Source can't be found.

We recently opt our repo into package source mapping do we need to add a new mapping? Where is this package supposed to come from?

nkolev92

Looks good to me.

The 1 or 2 changes that need addressed @zivkan can help with.

Don't block on me for merging.

dominoFire · 2022-02-15T18:32:01Z

@crummel 🔔

Do you need help?

crummel · 2022-02-15T21:22:04Z

That would be great. Sorry, I've been working on this on and off but having some problems. Currently what I'm looking at is the
/home/chris/nuget-tarball-2/.dotnet/sdk/6.0.101/NuGet.targets(130,5): error : Invalid framework identifier ''. [/home/chris/nuget-tarball-2/src/nuget-client.d8e55212488236cb12610e6557f523d92cfa5d15/build/restorehelper.targets] error. This seems like the TargetFrameworks property is not being set correctly somewhere but it's hard to debug in the binlog because the restorehelper doesn't say which project specifically it's failing on. Any ideas?

nkolev92 · 2022-02-16T22:43:12Z

@crummel

It's hard to debug, but basically it's NuGet.Frameworks

There's probably something empty in there.

src/NuGet.Core/NuGet.Frameworks/NuGet.Frameworks.csproj

src/NuGet.Core/NuGet.CommandLine.XPlat/NuGet.CommandLine.XPlat.csproj

crummel · 2022-03-14T18:59:42Z

@nkolev92 @zivkan @dominoFire This passed an internal build (https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5876410&view=results) as well as a local build in the source-build overall. Let me know if there's any more changes to make. Thanks for your help and sorry I kept getting interrupted and dragged this out so long!

crummel · 2022-03-15T23:09:47Z

I don't have merge permissions so whenever you are happy with this it's good to go. Thanks again!

ghost added the Community PRs created by someone not in the NuGet team label Jan 13, 2022

crummel mentioned this pull request Jan 13, 2022

[main] Backport source-build patches to repos. dotnet/source-build#2708

Closed

crummel marked this pull request as ready for review January 24, 2022 19:47

crummel requested a review from a team as a code owner January 24, 2022 19:47

nkolev92 reviewed Jan 25, 2022

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/NuGet.CommandLine.XPlat.csproj Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRequestMessageExtensions.cs Outdated Show resolved Hide resolved

zivkan assigned dominoFire Jan 25, 2022

dominoFire added the Approved for CI label Jan 27, 2022

nkolev92 reviewed Jan 28, 2022

View reviewed changes

build/packages.targets Outdated Show resolved Hide resolved

MichaelSimons mentioned this pull request Feb 1, 2022

[release/7.0.1xx-preview1] Update dependencies from dotnet/sdk dotnet/installer#13147

Merged

zivkan reviewed Feb 1, 2022

View reviewed changes

zivkan added Approved for CI and removed Approved for CI labels Feb 1, 2022

nkolev92 reviewed Feb 1, 2022

View reviewed changes

src/NuGet.Core/NuGet.Configuration/NuGet.Configuration.csproj Show resolved Hide resolved

crummel force-pushed the sourceBuildPatches branch from 3edc826 to d8e5521 Compare February 2, 2022 20:08

zivkan previously approved these changes Feb 2, 2022

View reviewed changes

zivkan added Approved for CI and removed Approved for CI labels Feb 2, 2022

nkolev92 reviewed Feb 2, 2022

View reviewed changes

nkolev92 reviewed Feb 16, 2022

View reviewed changes

src/NuGet.Core/NuGet.Frameworks/NuGet.Frameworks.csproj Outdated Show resolved Hide resolved

zivkan reviewed Feb 18, 2022

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/NuGet.CommandLine.XPlat.csproj Show resolved Hide resolved

zivkan added Approved for CI and removed Approved for CI labels Feb 18, 2022

dominoFire added Approved for CI and removed Approved for CI labels Mar 1, 2022

nkolev92 dismissed zivkan’s stale review via 09639d8 March 11, 2022 21:14

crummel added 12 commits March 12, 2022 14:56

Re-enable fullfx TFM in NuGet.

adbf10e

Upgrade to new CommandLineUtils package.

9b73ce6

Allow for .NET6 SDK as well as .NET5.

0d21314

Fix double-parentheses.

24ef9df

Fix some build issues.

261155f

Update for code review comment.

4d08aee

Updated for review comment to prevent warning as error.

000e5c7

Address code review comments.

3e553fe

Fix bad copy-paste of TFM property.

a216314

Fix missing TFM

b06adf5

More TFM fixes.

ecb957b

Fix CommandLineUtils sources.

e45613c

crummel force-pushed the sourceBuildPatches branch from 09639d8 to e45613c Compare March 12, 2022 20:56

crummel added 2 commits March 13, 2022 16:28

Believe we also need the net472 build in source-build.

89da726

Try with the locally available MS.Ext.CommandLineUtils.Sources.

d23d7a9

nkolev92 approved these changes Mar 15, 2022

View reviewed changes

dominoFire approved these changes Mar 15, 2022

View reviewed changes

dominoFire merged commit b5f0bb7 into NuGet:dev Mar 15, 2022

nkolev92 mentioned this pull request Apr 18, 2022

Don't import reference assemblies for nestandard builds in NuGet.Configuration #4582

Merged

8 tasks

MichaelSimons mentioned this pull request May 24, 2022

Pin Microsoft.Build and Microsoft.Extensions.CommandLineUtils.Sources versions for source-build #4643

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply source-build patches #4404

Apply source-build patches #4404

crummel commented Jan 13, 2022 •

edited

Loading

nkolev92 left a comment

crummel commented Feb 1, 2022 •

edited

Loading

zivkan Feb 1, 2022

zivkan Feb 1, 2022

zivkan Feb 2, 2022

zivkan Feb 1, 2022

zivkan Feb 1, 2022

dominoFire commented Feb 1, 2022

zivkan commented Feb 2, 2022

nkolev92 left a comment

dominoFire commented Feb 15, 2022

crummel commented Feb 15, 2022

nkolev92 commented Feb 16, 2022

crummel commented Mar 14, 2022

crummel commented Mar 15, 2022

Apply source-build patches #4404

Apply source-build patches #4404

Conversation

crummel commented Jan 13, 2022 • edited Loading

Bug

Description

PR Checklist

nkolev92 left a comment

Choose a reason for hiding this comment

crummel commented Feb 1, 2022 • edited Loading

zivkan Feb 1, 2022

Choose a reason for hiding this comment

zivkan Feb 1, 2022

Choose a reason for hiding this comment

zivkan Feb 2, 2022

Choose a reason for hiding this comment

zivkan Feb 1, 2022

Choose a reason for hiding this comment

zivkan Feb 1, 2022

Choose a reason for hiding this comment

dominoFire commented Feb 1, 2022

zivkan commented Feb 2, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

dominoFire commented Feb 15, 2022

crummel commented Feb 15, 2022

nkolev92 commented Feb 16, 2022

crummel commented Mar 14, 2022

crummel commented Mar 15, 2022

crummel commented Jan 13, 2022 •

edited

Loading

crummel commented Feb 1, 2022 •

edited

Loading