-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update branding to 6.0.111 #28322
Update branding to 6.0.111 #28322
Conversation
Looks like it's in need of some baseline updates. Taking a look... |
I've been trying to update the baselines, but not getting the expected results. Investigating further. |
@TanayParikh I assume that #28346 is an attempt at fixing the tests for the branding updates? Looks like it's still failing at the moment. Why would branding break this? |
Yes indeed. Still failing, I just pushed another commit a short while back which should hopefully give us a bit more diagnostic information.
It really shouldn't, but the warning messages are those of a baseline conflict. Maybe some other update went through which resulted in these failures? cc/ @javiercn in case Javier has additional context. |
Benchmark changes not having an impact. @javiercn any ideas on what may be going on here? |
Spoke with @javiercn offline. It looks like there could've been some changes to the project config, and missing RIDs which could be causing this. It's very unclear why this would be impacted by branding however. |
@TanayParikh should we consider disabling these tests temporarily while a fix is investigated? |
How urgently do we need to get this (and the other servicing PRs) in? I've been looking into this but haven't had much luck yet. |
We'll be generating builds next week for 6.0 but there are a few PRs backed up waiting on branding to go in first afaik. Looks like ~5 PRs waiting for the branches to open: https://github.com/dotnet/sdk/pulls?q=is%3Apr+is%3Aopen+base%3Arelease%2F6 (the middle five from revert through the linker update) |
Got it, thanks for the clarification. In that case, should things not be resolved by mid-day tomorrow, let's disable these tests temporarily. Would that be acceptable? |
Looks like we may actually be running into https://github.com/dotnet/sdk/blob/release/7.0.1xx-rc1/documentation/general/SelfContainedBreakingChangeNotification.md. However that's a 7.0 change, so not sure why it would have an impact here. cc/ @dsplaisted in case there's some additional context we may be missing. |
@TanayParikh The change had parts both in MSBuild and in the SDK. The MSBuild changes weren't supposed to change any behavior without the SDK changes. However, we had a bug which is probably why you are seeing these test failures in the full framework legs. It was fixed for GA in this PR: dotnet/msbuild#8002 So it looks like the test failures are due to a regression in Full Framework MSBuild behavior which will be fixed for 17.4 GA. |
@dsplaisted the sdk we're using to build 1xx, 3xx, and 4xx is all 6.0 based SDKs that shouldn't have the msbuild change (which as 17.4 only), correct? So is the issue here that we're using prerelease helix queues so the tests are actually running against 17.4 previews rather than 17.3 and earlier? Line 94 in 573ebc6
|
I went ahead and switched from the .pre images which should be 17.3.3 to see if that'll fix this PR. |
Looks like there are some infra issues (403s), and some other tests failing. But the razor/blazor tests are working now. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@TanayParikh I spoke to dnceng and while there are build images with 17.3, there are no test images. As such, I think the best path forward is to disable these tests temporarily. Any concerns? |
#28430 Here's an attempt and branding and test disablement. If this works, port to the other two branches. |
@dsplaisted to confirm, this regression shouldn't have any product experience impact? |
There was a regression in a preview of MSBuild, which is what is causing the tests to fail. It will be fixed in the GA release. |
No description provided.