Move BuildRID lists to common props #55279

am11 · 2021-07-07T16:37:33Z

Platform matrix testing is in progress, but locally made sure that the artifacts/package directory tree on macOS is identical before and after this change.

This PR is simplifying the BuildRID part. It removes OfficialBuildRID and UnofficialBuildRID (not to be confused with very similar-looking OfficialBuildID which is a contract between runtime and arcade for versioning). Also fixes tizen/armel crossgen2 issue at the same time.

cc @ViktorHofer

dotnet-issue-labeler · 2021-07-07T16:37:37Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ViktorHofer · 2021-07-07T17:38:53Z

@dotnet/runtime-infrastructure and @ericstj as I'm out the next two weeks, can you please help @am11 by reviewing and validating the change? We need official build validation for this change as well. Thanks

am11 · 2021-07-08T18:21:06Z

Remaining failures are unrelated to the change (NuGet hangups, EventSource test failures). Marking it as ready for review.

safern

Looks reasonable to me, but I would like either @hoyosjs or @jkoritzinsky to take another look as well.

Also, do we need to test this in an official build?

hoyosjs · 2021-07-08T20:00:30Z

I'll review and kick off an internal build. We should also test this for sourcebuild.

am11 · 2021-07-08T20:17:58Z

Thanks. There was a redundant exclusion I have just deleted (it won't change the outcome since RestoreBuildRID is used in a place where ExcludeFromBuildRID is not set).

am11 · 2021-07-09T21:34:59Z

@hoyosjs, was the internal build green?

We should also test this for sourcebuild.

I can see Build Linux x64 release SourceBuild is green in runtime-dev-innerloop.

hoyosjs · 2021-07-10T09:28:04Z

I did a manual check with non-portable rids for sourcebuild and all looked ok. I also code reviewed and it looks fine. I kicked off one and will report when I see it finish.

hoyosjs · 2021-07-10T17:19:29Z

Looks like there are some repeated assets.

D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.linux-arm.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.linux-arm.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.linux-arm64.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.linux-arm64.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.linux-musl-x64.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.linux-musl-x64.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.linux-x64.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.linux-x64.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.osx-x64.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'runtime.osx-x64.runtime.native.System.IO.Ports' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.linux-arm.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.linux-arm.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.linux-arm64.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.linux-arm64.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.linux-musl-x64.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.linux-musl-x64.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.linux-x64.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.linux-x64.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
D:\a\1\s\.packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.osx-x64.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2' 
##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21330.2\tools\SdkTasks\PublishBuildAssets.proj(43,5): error : Repeated Asset entry: 'assets/symbols/runtime.osx-x64.runtime.native.System.IO.Ports.6.0.0-preview.7.21360.2.symbols.nupkg' - '6.0.0-preview.7.21360.2'

am11 · 2021-07-10T18:29:41Z

Can we run that target locally or is it something only run on official build? If there is no secure argument involved please paste the command, I can try to investigate it locally.

hoyosjs · 2021-07-10T18:32:57Z

It means that two different legs produced it. Currently trying to see which two legs produced the same package.

hoyosjs · 2021-07-10T20:27:50Z

Looks like Libraries Build Linux x64 Release is producing it and at the same time Build Linux x64 release AllSubsets_Mono leg and Build Linux x64 release AllSubsets_Mono_LLVMJIT are also producing it and that caused the error - and similarly for all other repeated packages.

The command line for the libraries

./build.sh -subset libs -rc release -configuration Release -ci -arch x64 -framework net6.0 /p:OfficialBuildId=20210710.2

And for the allsubsets one:

./build.sh -ci -arch x64 -os Linux -s mono+libs+host+packs+mono.mscordbi -c Release /p:OfficialBuildId=20210710.2 /p:CrossBuild=false /p:MonoLLVMUseCxx11Abi=false

Another difference, we are now suddenly producing these that we weren't:

    <Package Id="runtime.osx-arm64.Microsoft.NETCore.ILAsm" Version="6.0.0-preview.7.21360.2" />
    <Package Id="runtime.osx-arm64.Microsoft.NETCore.ILDAsm" Version="6.0.0-preview.7.21360.2" />
    <Package Id="runtime.osx-arm64.Microsoft.NETCore.TestHost" Version="6.0.0-preview.7.21360.2" />

hoyosjs · 2021-07-10T20:31:53Z

src/libraries/pkg/runtime.native.System.IO.Ports/runtime.native.System.IO.Ports.pkgproj

@@ -4,7 +4,7 @@
    <SkipPackageFileCheck>true</SkipPackageFileCheck>
    <SkipValidatePackage>true</SkipValidatePackage>
  </PropertyGroup>
-  <ItemGroup Condition="'$(PackageTargetRuntime)' == 'linux-arm' or '$(PackageTargetRuntime)' == 'linux-arm64' or '$(PackageTargetRuntime)' == 'linux-x64' or '$(PackageTargetRuntime)' == 'osx-x64' or '$(PackageTargetRuntime)' == 'freebsd-x64'">
+  <ItemGroup Condition="'$(PackageTargetRuntime)' != '' and !$(PackageTargetRuntime.ToLowerInvariant.StartsWith('win'))">


This somehow didn't consider ARM64 before. It also now considers linux-musl variants when it didn't before.

ARM64 is present before and after the change. linux-musl is not (that restriction does not seem intentional either). I think this is not causing the validation error. I think the issue is that we have three ways of including pkgproj, one from src/libraries/libraries-packages, one from .proj file and then again from Directory.Build.props. We should try to simplify it, so transition to SDK/dotnet-pack in future become easier.

In this PR, I have changed complex whitelisting to "require a flat list and exclude" mechanism.

osx-arm64 I mean

I am not saying I am against it. I am just saying we might start seeing assets we didn't see before. I'll all for a single list of support and then projects that need can opt out.

hoyosjs · 2021-07-10T20:38:29Z

src/coreclr/.nuget/Directory.Build.props

-    </OfficialBuildRID>
-  </ItemGroup>
-  <ItemGroup Condition="$(SupportedPackageOSGroups.Contains(';OSX;'))">
-    <OfficialBuildRID Include="osx-x64" />


Probably this is why we didn't have arm64 ILAsm

into feature/consolidations

hoyosjs · 2021-07-12T21:45:28Z

I still see some manifest differences. I'll try later.

hoyosjs · 2021-07-13T08:17:29Z

I checked this again and the following packages are missing now compared to tip of main:

<Package Id="runtime.linux-arm.runtime.native.System.IO.Ports" Version="6.0.0-preview.7.21362.14" />
<Package Id="runtime.linux-arm64.runtime.native.System.IO.Ports" Version="6.0.0-preview.7.21362.14" />
<Package Id="runtime.linux-x64.runtime.native.System.IO.Ports" Version="6.0.0-preview.7.21362.14" />
<Package Id="runtime.osx-arm64.Microsoft.NETCore.ILAsm" Version="6.0.0-preview.7.21362.14" />
<Package Id="runtime.osx-arm64.Microsoft.NETCore.ILDAsm" Version="6.0.0-preview.7.21362.14" />
<Package Id="runtime.osx-arm64.Microsoft.NETCore.TestHost" Version="6.0.0-preview.7.21362.14" />
<Package Id="runtime.osx-x64.runtime.native.System.IO.Ports" Version="6.0.0-preview.7.21362.14" />

but they are referenced by the metapackage runtime.native.System.IO.Ports, and a bunch of unofficial rids that we don't produce packages for:

Meanwhile this is the dep list for the one in main:

This would probably break things. Also @ericstj not sure who owns ports, but shouldn't the package also exist in main for ARM64 osx? Although to be fair, the musl variants seem not to have one so it might be OK?

am11 · 2021-07-13T09:09:06Z

Lets start it at some other point, since I don't have time to further work on it. Changing one flag breaks some superfluous check somewhere which we can't locally reproduce, seems like we first need to fix CI to cover all sanity checks before doing refactoring.

hoyosjs · 2021-07-13T09:25:20Z

Yeah, this is pretty sad. But there's some place where things are more coupled than we realize. In this case the official build even was green and so was CI - so we are even missing some deeper validation.

ericstj · 2021-07-13T16:40:41Z

Also @ericstj Eric St. John FTE not sure who owns ports, but shouldn't the package also exist in main for ARM64 osx? Although to be fair, the musl variants seem not to have one so it might be OK?

@krwq @wfurt and originally worked on it, I think this falls under @dotnet/area-system-io.

wfurt · 2021-07-13T17:24:26Z

as far as I remember we did the extra IO.Ports packages so one can consume them on older versions. While consistency is nice I'm not sure there is real demand since the Serial port is not available for quite a while and it should work when used with current runtime. In some cases like ARM macOS the older versions did not even function on that platform.

ericstj · 2021-07-13T17:35:19Z

as far as I remember we did the extra IO.Ports packages so one can consume them on older versions

It seems to be used on all frameworks, unix runtimes today, which would include OSX ARM64:

runtime/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj

Lines 116 to 119 in b056b7b

    
           <Compile Include="$(CommonPath)Interop\Unix\System.IO.Ports.Native\Interop.Termios.cs" 
        
                    Link="Common\Interop\Unix\System.IO.Ports.Native\Interop.Termios.cs"/> 
        
           <Compile Include="$(CommonPath)Interop\Unix\System.IO.Ports.Native\Interop.Serial.cs" 
        
                    Link="Common\Interop\Unix\System.IO.Ports.Native\Interop.Serial.cs"/>

It'd be great if that could be removed on latest frameworks.

clamp03 · 2021-07-16T00:13:49Z

@am11 Tizen/armel build is still broken. If you don't mind, could I make a new PR to fix Tizen/armel crossgen2 issue based on this PR?

am11 · 2021-07-16T00:49:32Z

@clamp03, I am very sorry. I totally forgot about Tizen fix. 🤦
Opened a PR with just those parts: #55789.

am11 force-pushed the feature/consolidations branch from 68cc90f to 52e4a40 Compare July 7, 2021 16:51

am11 force-pushed the feature/consolidations branch 2 times, most recently from d093d23 to 56ee8e3 Compare July 8, 2021 15:08

am11 marked this pull request as ready for review July 8, 2021 18:21

am11 force-pushed the feature/consolidations branch from 56ee8e3 to e393b6a Compare July 8, 2021 19:30

Move BuildRID lists to common props

9078645

am11 force-pushed the feature/consolidations branch from e393b6a to 9078645 Compare July 8, 2021 19:31

safern approved these changes Jul 8, 2021

View reviewed changes

Delete a redundant exclusion

3c0265f

hoyosjs self-requested a review July 8, 2021 20:25

runfoapp bot mentioned this pull request Jul 10, 2021

Feed unreliability affecting CI #55449

Closed

hoyosjs added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 10, 2021

hoyosjs reviewed Jul 10, 2021

View reviewed changes

am11 and others added 4 commits July 12, 2021 03:14

Move RequireBuildRid to pkgproj

b152d62

Merge dotnet/main into feature/consolidations

41247ad

Merge remote-tracking branch 'origin/main' into feature/consolidations

d34f427

Move BuildRID lists to common props

7e03c4c

am11 force-pushed the feature/consolidations branch from 41247ad to 7e03c4c Compare July 12, 2021 09:01

Merge branch 'feature/consolidations' of https://github.com/am11/runtime

ebe3f1a

into feature/consolidations

ericstj requested a review from jkoritzinsky July 12, 2021 21:41

jkoritzinsky approved these changes Jul 12, 2021

View reviewed changes

runfoapp bot mentioned this pull request Jul 13, 2021

system.linq.expressions.tests work item #55546

Closed

am11 closed this Jul 13, 2021

am11 deleted the feature/consolidations branch July 13, 2021 09:09

runfoapp bot mentioned this pull request Jul 15, 2021

System.Net.Requests.Tests failing with a crash #55714

Closed

ghost locked as resolved and limited conversation to collaborators Aug 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move BuildRID lists to common props #55279

Move BuildRID lists to common props #55279

am11 commented Jul 7, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Jul 7, 2021

ViktorHofer commented Jul 7, 2021

am11 commented Jul 8, 2021

safern left a comment

hoyosjs commented Jul 8, 2021

am11 commented Jul 8, 2021

am11 commented Jul 9, 2021

hoyosjs commented Jul 10, 2021

hoyosjs commented Jul 10, 2021

am11 commented Jul 10, 2021

hoyosjs commented Jul 10, 2021

hoyosjs commented Jul 10, 2021 •

edited

Loading

hoyosjs Jul 10, 2021

am11 Jul 10, 2021 •

edited

Loading

hoyosjs Jul 10, 2021

hoyosjs Jul 10, 2021

hoyosjs Jul 10, 2021

hoyosjs commented Jul 12, 2021

hoyosjs commented Jul 13, 2021

am11 commented Jul 13, 2021

hoyosjs commented Jul 13, 2021

ericstj commented Jul 13, 2021

wfurt commented Jul 13, 2021

ericstj commented Jul 13, 2021

clamp03 commented Jul 16, 2021

am11 commented Jul 16, 2021

Move BuildRID lists to common props #55279

Move BuildRID lists to common props #55279

Conversation

am11 commented Jul 7, 2021 • edited Loading

dotnet-issue-labeler bot commented Jul 7, 2021

ViktorHofer commented Jul 7, 2021

am11 commented Jul 8, 2021

safern left a comment

Choose a reason for hiding this comment

hoyosjs commented Jul 8, 2021

am11 commented Jul 8, 2021

am11 commented Jul 9, 2021

hoyosjs commented Jul 10, 2021

hoyosjs commented Jul 10, 2021

am11 commented Jul 10, 2021

hoyosjs commented Jul 10, 2021

hoyosjs commented Jul 10, 2021 • edited Loading

hoyosjs Jul 10, 2021

Choose a reason for hiding this comment

am11 Jul 10, 2021 • edited Loading

Choose a reason for hiding this comment

hoyosjs Jul 10, 2021

Choose a reason for hiding this comment

hoyosjs Jul 10, 2021

Choose a reason for hiding this comment

hoyosjs Jul 10, 2021

Choose a reason for hiding this comment

hoyosjs commented Jul 12, 2021

hoyosjs commented Jul 13, 2021

am11 commented Jul 13, 2021

hoyosjs commented Jul 13, 2021

ericstj commented Jul 13, 2021

wfurt commented Jul 13, 2021

ericstj commented Jul 13, 2021

clamp03 commented Jul 16, 2021

am11 commented Jul 16, 2021

am11 commented Jul 7, 2021 •

edited

Loading

hoyosjs commented Jul 10, 2021 •

edited

Loading

am11 Jul 10, 2021 •

edited

Loading