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

[release/8.0] Manually update release/8.0 arcade to latest #92778

Merged
merged 8 commits into from
Oct 6, 2023

Conversation

lewing
Copy link
Member

@lewing lewing commented Sep 28, 2023

Arcade flow is stuck somewhere in release/8.0 this the result of running darc update-dependencies --id 194643 manually.

cc @ViktorHofer @mmitche

@ghost
Copy link

ghost commented Sep 28, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Arcade flow is stuck somewhere in release/8.0 this the result of running darc update-dependencies --id 194643 manually.

cc @ViktorHofer

Author: lewing
Assignees: lewing
Labels:

area-Infrastructure-libraries

Milestone: -

@lewing
Copy link
Member Author

lewing commented Sep 28, 2023

cc @ericstj

@lewing
Copy link
Member Author

lewing commented Sep 28, 2023

@sbomer I assume this is fixed via dotnet/sdk#35767 ?

@sbomer
Copy link
Member

sbomer commented Sep 28, 2023

No, dotnet/sdk#35767 won't fix this instance because it was caused by setting <IsTrimmable>true</IsTrimmable> for a netstandard2.0-only project.

@sbomer
Copy link
Member

sbomer commented Sep 28, 2023

Now seeing errors like

##[error]src/coreclr/tools/Common/TypeSystem/Ecma/SymbolReader/UnmanagedPdbSymbolReader.cs(287,67): error CS9191: (NETCORE_ENGINEERING_TELEMETRY=Build) The 'ref' modifier for argument 2 corresponding to 'in' parameter is equivalent to 'in'. Consider using 'in' instead.

@ghost
Copy link

ghost commented Sep 28, 2023

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Arcade flow is stuck somewhere in release/8.0 this the result of running darc update-dependencies --id 194643 manually.

cc @ViktorHofer @mmitche

Author: lewing
Assignees: lewing
Labels:

blocking-release, area-Host, area-codeflow

Milestone: -

@lewing
Copy link
Member Author

lewing commented Sep 28, 2023

@elinor-fung are the host failures here rid related?

@@ -1,16 +1,16 @@
{
"sdk": {
"version": "8.0.100-preview.7.23376.3",
"version": "8.0.100-rc.1.23455.8",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lewing Should this be rtm? Also the one below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking because we've been doing similar manual adjustments in wasm flows. I don't know if it applies to this as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, we have to wait until we have an RTM SDK. I believe the arcade insertion is just doing it's thing propagating the arcade toolset changes here.

Copy link
Member Author

@lewing lewing Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need a newer sdk to be able to take the new roslyn in #92503 (comment) but this pr is just to catch us up with normal arcade flow

@carlossanlop carlossanlop added the Servicing-approved Approved for servicing release label Sep 28, 2023
@ericstj
Copy link
Member

ericstj commented Sep 28, 2023

I noticed we took the RC toolset update in 02bcb36. Among those other changes might be things needed here to update the toolset.

@agocke
Copy link
Member

agocke commented Sep 28, 2023

The problem is the src/tests/Common/external/external.csproj project. It specifies win7-x64 as a TFM. But it's even stranger because the comment says the RID doesn't even matter.

The whole thing smells of giant hacks, but I bet updating to win-x64 will fix the problem, even though I don't really know what's going on.

@elinor-fung
Copy link
Member

elinor-fung commented Sep 28, 2023

The whole thing smells of giant hacks, but I bet updating to win-x64 will fix the problem, even though I don't really know what's going on.

This - win-x86/win-x64 - is what we have in main. (I'm also unclear as to what it is doing or what that comment means.)

@ericstj
Copy link
Member

ericstj commented Sep 28, 2023

We don't have to do it in this PR, but I bet we can remove that RID. It looks to me like this dates back to 1.1 days where we had implementations in RID-specific packages. So in order to get NuGet to restore the right stuff to copy a RID that plugged into the graph was needed. b843687#diff-61c4029d873ebe0ce8a16d30a697127a7c96e11a1cc018936df1963aba0ef40bR12

@ViktorHofer
Copy link
Member

The problem is the src/tests/Common/external/external.csproj project. It specifies win7-x64 as a TFM. But it's even stranger because the comment says the RID doesn't even matter.
The whole thing smells of giant hacks, but I bet updating to win-x64 will fix the problem, even though I don't really know what's going on.

AFAIK, after the runtime tests are all migrated to the new source generated model (which should be completed soon), they will participate in NuGet restore and will get their dependencies directly, instead of via the external.csproj project. That project and setting a RID for restore will then go away.

@lewing
Copy link
Member Author

lewing commented Sep 29, 2023

So installer rid resolution failures left (other than CI noise)?

@carlossanlop
Copy link
Member

@lewing @ViktorHofer do we need to address those rid-related CI failures in this PR, or should I open a separate issue to get them fixed?

@ViktorHofer
Copy link
Member

Need to be addressed here. Those are real failures.

@elinor-fung
Copy link
Member

_RequiresILLinkPack ended up being true for packaging projects based on the single-file analyzer being enabled. As a result, this was trying to process framework references (it didn't before), which ended up adding those packages as download dependencies. https://github.com/dotnet/sdk/blob/4a1ffab37158144f8b1bc5995241b7ace7aa19ea/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L63

Last commit ports over the below from main:

<PropertyGroup Condition="'$(UsingMicrosoftNoTargetsSdk)' == 'true' or
'$(UsingMicrosoftDotNetSharedFrameworkSdk)' == 'true' or
'$(MSBuildProjectExtension)' == '.pkgproj' or
'$(UsingMicrosoftTraversalSdk)' == 'true'">
<!-- Explicitly disable running analyzers to avoid trying to discover the correct ILLink tool pack for a project that has no sources. -->
<RunAnalyzers>false</RunAnalyzers>
</PropertyGroup>

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is green and the two remaining feedback items were addressed by @elinor-fung and @lewing. Merging now.

@carlossanlop carlossanlop merged commit 0c2796c into dotnet:release/8.0 Oct 6, 2023
189 checks passed
@lewing
Copy link
Member Author

lewing commented Oct 6, 2023

_RequiresILLinkPack ended up being true for packaging projects based on the single-file analyzer being enabled. As a result, this was trying to process framework references (it didn't before), which ended up adding those packages as download dependencies. https://github.com/dotnet/sdk/blob/4a1ffab37158144f8b1bc5995241b7ace7aa19ea/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L63

Last commit ports over the below from main:

<PropertyGroup Condition="'$(UsingMicrosoftNoTargetsSdk)' == 'true' or
'$(UsingMicrosoftDotNetSharedFrameworkSdk)' == 'true' or
'$(MSBuildProjectExtension)' == '.pkgproj' or
'$(UsingMicrosoftTraversalSdk)' == 'true'">
<!-- Explicitly disable running analyzers to avoid trying to discover the correct ILLink tool pack for a project that has no sources. -->
<RunAnalyzers>false</RunAnalyzers>
</PropertyGroup>

Many thanks for resolving that.

@lewing lewing deleted the update-arcade-r8 branch October 6, 2023 19:03
@ghost ghost locked as resolved and limited conversation to collaborators Nov 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-codeflow for labeling automated codeflow area-Infrastructure-installer blocking-release Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants