Skip to content

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

Merged
carlossanlop merged 8 commits into
dotnet:release/8.0from
lewing:update-arcade-r8
Oct 6, 2023
Merged

[release/8.0] Manually update release/8.0 arcade to latest#92778
carlossanlop merged 8 commits into
dotnet:release/8.0from
lewing:update-arcade-r8

Conversation

@lewing

@lewing lewing commented Sep 28, 2023

Copy link
Copy Markdown
Member

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

cc @ViktorHofer @mmitche

@ghost

ghost commented Sep 28, 2023

Copy link
Copy Markdown

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

lewing commented Sep 28, 2023

Copy link
Copy Markdown
Member Author

cc @ericstj

@lewing

lewing commented Sep 28, 2023

Copy link
Copy Markdown
Member Author

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

@sbomer

sbomer commented Sep 28, 2023

Copy link
Copy Markdown
Member

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

sbomer commented Sep 28, 2023

Copy link
Copy Markdown
Member

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

ghost commented Sep 28, 2023

Copy link
Copy Markdown

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

lewing commented Sep 28, 2023

Copy link
Copy Markdown
Member Author

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

Comment thread global.json
{
"sdk": {
"version": "8.0.100-preview.7.23376.3",
"version": "8.0.100-rc.1.23455.8",

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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.

@lewing lewing Sep 28, 2023

Copy link
Copy Markdown
Member Author

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

ericstj commented Sep 28, 2023

Copy link
Copy Markdown
Member

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

@agocke

agocke commented Sep 28, 2023

Copy link
Copy Markdown
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.

@elinor-fung

elinor-fung commented Sep 28, 2023

Copy link
Copy Markdown
Member

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

ericstj commented Sep 28, 2023

Copy link
Copy Markdown
Member

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

Comment thread src/coreclr/tools/aot/Directory.Build.props Outdated
@ViktorHofer

Copy link
Copy Markdown
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

lewing commented Sep 29, 2023

Copy link
Copy Markdown
Member Author

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

@carlossanlop

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Member

Need to be addressed here. Those are real failures.

@elinor-fung

Copy link
Copy Markdown
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>

@carlossanlop carlossanlop left a comment

Copy link
Copy Markdown
Contributor

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
@lewing

lewing commented Oct 6, 2023

Copy link
Copy Markdown
Member Author

_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.

8 participants