-
Notifications
You must be signed in to change notification settings - Fork 703
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
Apply arcade-powered source-build patches #4216
Conversation
Hey @crummel Can you please add desriptions for the all the changes and the motivation? |
@nkolev92 I updated the description. Let me know if you have any questions or want to chat about them. Thanks! |
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.
Thanks, the description made is super easy to follow @crummel
A couple of little things.
src/NuGet.Core/NuGet.Protocol/HttpSource/HttpRequestMessageExtensions.cs
Outdated
Show resolved
Hide resolved
f3a2d4d
to
046929c
Compare
In addition to addressing your comments I added a patch from Michael's work to update source-build to 6.0 RC, ("NuGet.Build.Tasks.Pack source-build support") which handles source-build missing ILMerge by including the individual DLLs instead of the merged one. Thanks for the review! |
@crummel sorry for the slow response/review. Looking at the CI logs, it failed the source-build, linux, and mac jobs because .NET 6 SDK isn't installed, hence it can't target net6.0. |
Sorry again for the slow review. The build still doesn't work:
This doesn't immediately fail the build, so it goes on to try to build with the system-installed
|
The source build job is still failing: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5258934&view=logs&j=cb98efd6-7062-56ef-f642-0176540bbe0a&t=d42c45d2-f6c8-5ae2-d854-abf6f8d78853
|
I don't know if it's intentional, but it's caused our linux test job to fail with the same error, .NET 6.0 SDK not installed. I'm guessing the Mac job would have failed as well, if/when it runs. Ideally since NuGet is only targeting the .NET 6.0 SDK now, this branch will never be inserted into .NET 5.0 SDK, we (someone in the NuGet team) should update the build. We just have too many other priorities at the moment. |
When I was reviewing the PR, I was under the impression that only source build should be setting net6.0. Maybe we're missing something? |
Yeah, my brain kept thinking about it after I posted while trying to do other work, and I don't think it's good for this source-build requirement to cause our other jobs/tests to start using a different TFM, as it can cause builds to break when new versions of .NET enable new code analyzers (and we use WarningsAsErrors, so it will break us), plus our dotnet.integration.tests project patches a .NET SDK with the current build of NuGet and runs some Therefore, I think it's a bug that the source-build TFM re-targeting to .NET 6 has "leaked" to our "normal" Linux and Mac CI jobs. |
I think the latest set of changes addresses your concerns with the Linux non-source-build build. Windows tests failed on the PR but I restarted them and I don't think it's likely that my changes are the cause - I'll keep an eye on them. |
Hi @zivkan, is this caused by the old copy of dotnet-install.sh we kept? Pls see #4289 (comment) for more details. |
@heng-liu no, the previous issue was caused by this PR changing the TFM for all "xplat" builds, rather than only source-build builds. However, there are now 9 tests on Mac failing, all of them to do with signed packages. I don't see how it's related to this PR's changes, but it looks like @crummel re-ran the Mac tests 7 times, and it keeps failing with the 9 signing tests, and our other builds are not failing these tests on Mac, so it does not appear to be an infrastructure issue. As the team's signed packages expert, can you take a look? |
Hi @zivkan and @crummel, sorry that I just saw this. The 9 tests on Mac are disabled in dev a few weeks ago (tracking issue: NuGet/Home#11178), so rebasing from dev should work. |
I rebased and that seems to have fixed the Mac tests. The Apex tests failed so I kicked those, but the VS EndToEnd test machine deployment is failing, any ideas? |
I see a build for your other PR, the BAR publishing, but not this PR. The first attempt for E2E tests failed for what I think is a Dartlab error. We already contacted the team, but haven't yet got a response. Attempts 2 onwards failed because you retried only the failed job, not the entire stage, and after the first attempt failed, the temporary assets were deleted, so trying to re-deploy the VM without running the configure VM job failed because the configuration file couldn't be found. |
CSC : warning AD0001: Analyzer 'Microsoft.CodeAnalysis.PublicApiAnalyzers.DeclarePublicApiAnalyzer' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'. CSC : error CS2001: Source file '/repos/installer2/artifacts/tarball/src/nuget-client.7363366401b43f4ea250394db8dad3707e9a9636/src/NuGet.Core/NuGet.Common/PublicAPI/net6.0/PublicAPI.Shipped.txt' could not be found.
- TargetFrameworks was actually blank in NuGet.Frameworks.csproj, changed it so that we set it. - Disable two obsoletion warnings in NuGet.Packaging.Test.csproj. - Explictly reference the correct ExecutionContext in TestNuGetProjectContext.
ad7f2dd
to
201e4e4
Compare
@zivkan What has NuGet been doing in regards to merging fixes to the release branch? Should I make a new PR to get this into release-6.0.x or will it get picked up automatically or is it too late? Thanks! |
@crummel I'll cherry-pick it into the release branch. We might not do any more insertions from that branch though, so the NuGet commit hash that corresponds to the binaries that ship in the .NET 6.0.100 SDK will not contain these changes. |
Bug
Fixes: NuGet/Home#11060
Fixes: dotnet/source-build#2311
Regression? Last working version:
Description
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation