nuget.client: build for .NET 5 #1828
Conversation
|
The build fails. A class is not found: Maybe the wrong tfm gets used: |
|
Hmm. Here are the binlogs from the build if needed: NuGet.Client.3501ddedc274ac10d4b135856b7593a6bb8a72f1.zip I think @crummel and @dseefeld have more recent experience here, with targetframework meddling... |
|
I'm not sure how to tackle this further. |
|
Looking at an rc1-based source-build I have, I see these in In Microsoft-built Since this NuGet project is targeting I think dotnet/runtime might be using package harvesting to get Moving NuGet forward to |
@dagood these packages get built from runtime repo, right? Is it by one of these build commands? Do you know which? I haven't tried figure it out.
In #1834 I'm trying the use |
Yes, this would be the On the other side, I confirmed with @crummel in our standup that changing NuGet.Client to target Having the projects automatically target the latest target framework feels wrong to me as a long-term solution (more about that at #1763). But flowing that in as a property is definitely something to consider as a way to get rid of patches if we end up needing to always target the latest framework for some reason. |
When I build the |
|
Well, System.Security.Cryptography.Cng.5.0.0-rc.1.20451.14.nupkg is created here in the PR build libraries binlog:
Maybe you could explain why you're trying to run this manually (presumably outside the context of source-build)? |
Thanks! Can I open these logs on Linux?
I can try things much faster by patching the repos directly. Maybe there are better ways to patch things in source-build, but I don't know those workflows. |
|
They get built conditionally on |
I've been able to build and run the binlog viewer on macOS with the Avalonia flavor: https://github.com/KirillOsenkov/MSBuildStructuredLog. There's also an online one but I have no idea if that'll work with the size of these logs. (From what I've seen the online one has a reduced feature set too.) I don't know if there's a better way to have a fast dev loop. In the past I've copied the environment lines from the default logs into a script that then re-runs the entry command. In theory the build is incremental enough that you could delete some patch + build semaphores to rerun what you want, but I haven't tried that. But why are you running dotnet/runtime? @dseefeld has noted there are problems when he tried that approach. |
|
The runtime libraries build only builds net5.0 packages right now. Discussions with the runtime team suggested that we could build using |
I've updated the PR to build for net5.0. CI has a hickup: |
|
One job hit that (opened #1839 to track as potential flakiness), but the others hit targetframework issues: This is the virality I mentioned in #1763, the downstreams need to compensate now and also build for |
| <VSThreadingVersion>16.6.13</VSThreadingVersion> | ||
| <!-- TODO - remove this when temporary patching is no longer necessary. See https://github.com/nuget/home/issues/8952 --> | ||
| - <PatchedSystemPackagesVersion>5.0.0-preview.3.20214.6</PatchedSystemPackagesVersion> | ||
| + <PatchedSystemPackagesVersion>5.0.0-rc.1.20451.14</PatchedSystemPackagesVersion> |
There was a problem hiding this comment.
@dagood this version probably needs an update by now. Is there a way to write this so it doesn't require patching?
There was a problem hiding this comment.
Strictly speaking to avoid the patch, this could be passed in as /p:PatchedSystemPackagesVersion=5.0.0 in the source-build proj, since global properties (that syntax) can't be overridden by this props file.
To avoid hard-coding a version, it could use a property imported from the generated PackageVersions.props file. It might be as simple as changing usages of PatchedSystemPackagesVersion to instead use the "actual" version props, like SystemSecurityCryptographyProtectedDataVersion, to pick up what dotnet/runtime built. This has plenty of nuance with how the projects actually evaluate in this specific case though. Not familiar enough with the repo to know more off the top of my head.
tmds
changed the title
|
The last CI run is before GA was merged, so I'd kind of expect merging this to cause the rolling build to fail. Not sure how to get GitHub to redo the CI merge other than pushing a commit to this PR. |
|
I've rebased on master. Let's see what CI makes of it. |
| <NETFXTargetFrameworkVersion>v4.7.2</NETFXTargetFrameworkVersion> | ||
| <NETFXTargetFramework>net472</NETFXTargetFramework> | ||
| - <NETCoreTargetFramework>netcoreapp2.1</NETCoreTargetFramework> | ||
| + <NETCoreTargetFramework>netcoreapp5.0</NETCoreTargetFramework> |
There was a problem hiding this comment.
Shouldn't this be net5.0? I thought netcoreapp5.0 is just an obsolete alias for that?
There was a problem hiding this comment.
The upstream repo hasn't adopted net5.0 yet, and some of the build logic relies on it being netcoreapp5.0.
|
@dagood what does it mean when _Production builds succeed, and _Online/_Offline don't? Looking at the logs for the failing builds makes me wonder if this patch was applied, for example: |
This means it's able to build using prebuilt dependencies and create the tarball (Production), but building the tarball (Online/Offline) didn't work. (Offline = internet feeds removed, Online = still there.) This is a pretty common state to be in. 😛
source-build/Directory.Build.props Line 23 in cb40d34
|
Ah right--this is kind of happening. The version of the NuGet packages being put into the build are the ones from the previously-source-built tarball, which were built in an earlier rolling build, so they don't have this patch. Online/Offline download a cached previously-source-built as an approximation of the N-1 => N flow. (A bootstrap build would have this patch, IIRC, because nupkgs built in stage 1 (Production) get fed into the tarball as previously-source-built.) I think the bit that's causing a problem here is that the prebuilt System.Security packages that we actually put in the tarball are based on the current Production build (with this patch), so we don't include them. However, previously-source-built packages still have their old dependencies from when they were originally built (without this patch), so now they can't find them. A few ideas:
The first seems the most doable + maintainable short term. It seems like reproing locally will be pretty important to reasonably work on that (since the build takes so long), I'd expect On the other hand, I do wonder if we could just toss these System.Security packages into SBRP. That usually seems to be more predictable and direct, even though moving to net5.0 does seem correct to me. 😕 |
I brought this up with the team and we think it makes sense for 5.0.0. Instead of downloading a previously-source-built.tar.gz, we'll pack up the nupkgs produced by the production build. I'm hopeful it's a tweak to the tarball generation code rather than something drastic. (We'll still produce previously-source-built.tar.gz files so that we can go back to a sort-of N-1 => N flow for 5.0.1.) Once I get that done, I expect this PR should work. (And this should let us remove those cyclic roslyn-analyzer prebuilts too. (#1878) 😄) |
|
@dagood thanks for the thorough explanation!
What artifacts does Online/Offline use from the previous source-build, and which come from the PR Production build? |
| <VSThreadingVersion>16.7.56</VSThreadingVersion> | ||
| <!-- TODO - remove this when temporary patching is no longer necessary. See https://github.com/nuget/home/issues/8952 --> | ||
| - <PatchedSystemPackagesVersion>5.0.0-preview.3.20214.6</PatchedSystemPackagesVersion> | ||
| + <PatchedSystemPackagesVersion>5.0.0</PatchedSystemPackagesVersion> |
There was a problem hiding this comment.
@dseefeld this change is for getting rid of the System.Security.Cryptography.Cng,5.0.0-preview.3.20214.6 and System.Security.Cryptography.Pkcs,5.0.0-preview.3.20214.6 prebuilts.
There was a problem hiding this comment.
Cool. Thanks. I updated the prebuilt wiki to include the PR and assigned you.
source-build/.vsts.pipelines/builds/matrix.yml Lines 109 to 114 in 25deb0c |
Merged #1881! Should be ready to try a merge/rebase of this PR and see if that helps. |
|
CI caught this issue with building nuget for net5.0 but arcade targeting netcoreapp2.1: (Copied from binlog to make it easier to read (IMO).) |
|
I guess this is being subsumed by #1890 which will address that. |
|
This was included in #1890. |
Use built version for System.Security.{Cryptography.Pkcs,ProtectedData,ProtectedData}
cc @dagood @crummel @dseefeld @omajid