Skip to content
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

[main] Rebootstrap with latest changes #19145

Merged
merged 19 commits into from
Mar 30, 2024
Merged

Conversation

github-actions[bot]
Copy link

@github-actions github-actions bot commented Mar 22, 2024

@github-actions github-actions bot requested review from a team as code owners March 22, 2024 17:20
@NikolaMilosavljevic
Copy link
Member

Will update with latest installer build to pick up newer publishing changes.

@NikolaMilosavljevic
Copy link
Member

Updated with SDK from the latest main build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2411912&view=results

@MichaelSimons
Copy link
Member

Updated with SDK from the latest main build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2411912&view=results

As a general rule, if the SDK is updated the PSBs should also be updated as well. This eliminates potential headaches caused by the mismatch.

@NikolaMilosavljevic
Copy link
Member

NikolaMilosavljevic commented Mar 22, 2024

Updated with SDK from the latest main build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2411912&view=results

As a general rule, if the SDK is updated the PSBs should also be updated as well. This eliminates potential headaches caused by the mismatch.

Yeah, working on it - using this build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2411637&view=results

@NikolaMilosavljevic
Copy link
Member

Hmm, failing to bootstrap, as SDK is not available - will need to update with a different build.

@NikolaMilosavljevic
Copy link
Member

NikolaMilosavljevic commented Mar 22, 2024

None of the newer installer builds are publishing assets. @ViktorHofer @mmitche

I've reverted to original PR changes.

@mthalman
Copy link
Member

None of the newer installer builds are publishing assets. @ViktorHofer @mmitche

I've reverted to original PR changes.

This is because of the 1ES template migration: #19016 (comment)

@ViktorHofer
Copy link
Member

Sounds like #19016 (comment) got resolved. Can we re-bootstrap with a newer SDK so that we include all the publishing changes?

@ViktorHofer
Copy link
Member

Submitted dotnet/winforms#11122 to fix the new analyzer warning. This could be patched to unblock this PR until the fix flows into installer.

@NikolaMilosavljevic
Copy link
Member

I've refreshed this PR with latest builds (from this morning). I've updated the first comment to reflect the asset sources.

Validation build is still running (stage 2 legs) - everything is green so far.

@MichaelSimons
Copy link
Member

@NikolaMilosavljevic - Please create a patch for @ViktorHofer dotnet/winforms#11122 to unblock this bootstrap. I'd like to get our full build green again.

@NikolaMilosavljevic
Copy link
Member

@NikolaMilosavljevic - Please create a patch for @ViktorHofer dotnet/winforms#11122 to unblock this bootstrap. I'd like to get our full build green again.

Sure - will do. Source-build validation was all green: https://dev.azure.com/dnceng/internal/_build/results?buildId=2414987&view=results

@ViktorHofer
Copy link
Member

ViktorHofer commented Mar 26, 2024

I think that dotnet/sdk#39782 already contains the fix. This might just be a matter of waiting for another 2 hours until the change is merged into installer main. Just saying, as resolving patches also adds noise to dependency flow.

@MichaelSimons
Copy link
Member

MichaelSimons commented Mar 26, 2024

I think that dotnet/sdk#39782 already contains the fix. This might just be a matter of waiting for another 2 hours until the change is merged into installer main. Just saying, as resolving patches also adds noise to dependency flow.

Two hours is pretty optimistic IMO. I do hear you though on resolving patches. Given this is blocking other work, my preference is to merge this PR and have be proactive on removing the patch in the dependency flow.

@NikolaMilosavljevic
Copy link
Member

I think that dotnet/sdk#39782 already contains the fix. This might just be a matter of waiting for another 2 hours until the change is merged into installer main. Just saying, as resolving patches also adds noise to dependency flow.

I don't think SDK flow contains the fix. Winforms did not flow to WindowsDesktop in couple weeks.

@ViktorHofer
Copy link
Member

Oh wow that would be bad. Is there a tracking issue for that?

@NikolaMilosavljevic
Copy link
Member

Oh wow that would be bad. Is there a tracking issue for that?

It seems that winforms flows through wpf - last update was exactly one week ago: dotnet/windowsdesktop#4260

@NikolaMilosavljevic
Copy link
Member

Added two more patches - for xdt and vstest.

@NikolaMilosavljevic
Copy link
Member

NikolaMilosavljevic commented Mar 29, 2024

Sigh... few more of the same in sdk - working on it.

##[error]/vmr/src/sdk/src/RazorSdk/Tool/Client.cs(172,31): error CA2022: (NETCORE_ENGINEERING_TELEMETRY=Build) Avoid inexact read with 'System.IO.Stream.ReadAsync(byte[], int, int, System.Threading.CancellationToken)' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2022)
##[error]/vmr/src/sdk/src/RazorSdk/Tool/ConnectionHost.cs(110,31): error CA2022: (NETCORE_ENGINEERING_TELEMETRY=Build) Avoid inexact read with 'System.IO.Stream.ReadAsync(byte[], int, int, System.Threading.CancellationToken)' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2022)

@NikolaMilosavljevic
Copy link
Member

Sigh... few more of the same in sdk - working on it.

##[error]/vmr/src/sdk/src/RazorSdk/Tool/Client.cs(172,31): error CA2022: (NETCORE_ENGINEERING_TELEMETRY=Build) Avoid inexact read with 'System.IO.Stream.ReadAsync(byte[], int, int, System.Threading.CancellationToken)' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2022)
##[error]/vmr/src/sdk/src/RazorSdk/Tool/ConnectionHost.cs(110,31): error CA2022: (NETCORE_ENGINEERING_TELEMETRY=Build) Avoid inexact read with 'System.IO.Stream.ReadAsync(byte[], int, int, System.Threading.CancellationToken)' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2022)

Updated the PR with sdk patch.

@NikolaMilosavljevic
Copy link
Member

One instance in nuget.client:

/vmr/src/nuget-client/src/NuGet.Core/NuGet.Packaging/Signing/Archive/SignedPackageArchiveUtility.cs(107,13): error CA2022: Avoid inexact read with 'System.IO.Stream.Read(byte[], int, int)' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2022) [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Packaging/NuGet.Packaging.csproj::TargetFramework=net9.0]

@NikolaMilosavljevic
Copy link
Member

Pushed a patch for nuget.client error. Unified build was fine, so hopefully this is the last error blocking the PR.

@NikolaMilosavljevic
Copy link
Member

Pushed a patch for nuget.client error. Unified build was fine, so hopefully this is the last error blocking the PR.

Messed up the patch - will redo.

@NikolaMilosavljevic
Copy link
Member

Hmm, interesting - another failure in nuget.client - peeling the onion...

@NikolaMilosavljevic
Copy link
Member

Pushed the fix. In total there are 2 CA2022 errors in nuget.client:

/vmr/src/nuget-client/src/NuGet.Core/NuGet.Packaging/Signing/Archive/SignedPackageArchiveUtility.cs(107,13): error CA2022: Avoid inexact read with 'System.IO.Stream.Read(byte[], int, int)' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2022) [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Packaging/NuGet.Packaging.csproj::TargetFramework=net9.0]
/vmr/src/nuget-client/src/NuGet.Core/NuGet.ProjectModel/Utf8JsonStreamReader.cs(55,13): error CA2022: Avoid inexact read with 'System.IO.Stream.Read(byte[], int, int)' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2022) [/vmr/src/nuget-client/src/NuGet.Core/NuGet.ProjectModel/NuGet.ProjectModel.csproj::TargetFramework=net9.0]

@MichaelSimons MichaelSimons merged commit 8e988a5 into main Mar 30, 2024
22 checks passed
@MichaelSimons MichaelSimons deleted the backport/pr-19085-to-main branch March 30, 2024 01:44
@ViktorHofer
Copy link
Member

ViktorHofer commented Apr 3, 2024

@NikolaMilosavljevic do you plan to send PRs into the five repositories so that we can remove the patches? I think we should do that.

@NikolaMilosavljevic
Copy link
Member

@NikolaMilosavljevic do you plan to send PRs into the five repositories so that we can remove the patches? I think we should do that.

I could only do PRs with the same exact changes. The goal was to let repos implement a proper fix, instead of temporarily disable the warning/error.

@ViktorHofer
Copy link
Member

I fear that repo owners won't see or get to those issues until they are actually blocked by them (when they upgrade to a recent SDK). That would probably mean having the patches around for two months which is undesirable given that we don't want to rely on patches anymore. I'm not saying that you should do the work, I just wonder how we can get rid of the patches asap.

@NikolaMilosavljevic
Copy link
Member

Not a problem to create PRs, as I already have all changes in their repos locally - used to create the patches. Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants