For 5.0.0, use bootstrapping in CI, not N-1 by dagood · Pull Request #1881 · dotnet/source-build

dagood · 2020-11-12T23:03:14Z

Instead of downloading previously-source-built packages (prev-sb) from some N-1 build, use the tar.gz that the Production build built. This simulates a bootstrapping flow.

For #1880

dagood · 2020-11-13T18:56:39Z

This is the situation for the System.Collections.Immutable/5.0.0-preview.8.20407.11 and System.Reflection.Metadata/5.0.0-preview.8.20407.11 prebuilts being detected. A little complicated due to arcade -> roslyn ordering and the cycles involved:

One fix is to override the Production dotnet/roslyn build to use a 5.0.0 prebuilt rather than 5.0.0-preview.8.20407.11. This would make the dependencies naturally resolve to the right thing in the Offline build because the version numbers match with a package we source-built. (Package restore hits the local source first, so it'll end up resolving the one we built.)
- But this doesn't seem futureproof, I don't know how to get the value of 5.0.0 in the Production build other than hard-coding. Ideally it'll match the version of System.Collections.Immutable we end up building in dotnet/runtime.
- This will also be impossible for arcade-powered source-build. Since builds will happen alongside the Microsoft build, we wouldn't actually be able to access the 5.0.0 prebuilt because it hasn't been produced yet.
Another way is adding a direct dependency into dotnet/arcade, rather than relying on Microsoft.CodeAnalysis.Common to have the right dependency. The Offline build can override the version via PVP to the 5.0.0 version that's actually available. (Microsoft.CodeAnalysis.Common/3.8.0 would still have a dependency on 5.0.0-preview.8.20407.11 but we override it with a more direct dependency from the project itself.) This seems like it'll work longer, but still involves patching dotnet/arcade.
- Going with this I think. (And I think this may be applied to more things in the future as a general approach.)
Something a little more wild would be modifying the Production-built Microsoft.CodeAnalysis.Common/3.8.0 nupkg itself before the Offline build to redirect its dependencies so they point at packages we actually built. This would involve no patches to the repos, but might be too chaotic to work out. Not sure it's worth giving this any more thought for now.
Yet another way is to restrict which packages are available, and tell NuGet to automatically upgrade 5.0.0-preview.8.20407.11 to 5.0.0 if it can't find 5.0.0-preview.8.20407.11. (Normally it emits a warning/error when it does this.) I'm not a fan of this because this means it becomes a floating-version dependency, with all those caveats.

Microsoft.NETCore.Platforms/2.1.2 is not in a project.assets.json and I haven't looked into it yet:

#> grep -i Microsoft.NETCore.Platforms/2.1.2 -r src/
src/sdk.0cdbf80b4176ba4b979bdfc7b630b884f656072f/artifacts/bin/Release/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json:      "Microsoft.NETCore.Platforms/2.1.2": {},
src/sdk.0cdbf80b4176ba4b979bdfc7b630b884f656072f/artifacts/bin/Release/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json:    "Microsoft.NETCore.Platforms/2.1.2": {
src/sdk.0cdbf80b4176ba4b979bdfc7b630b884f656072f/artifacts/bin/Release/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json:      "path": "microsoft.netcore.platforms/2.1.2",

dseefeld · 2020-11-13T19:16:23Z

I think Option 2 makes sense. I'm assuming the direct dependency would only be added in the offline build? Regarding how to find the version for Option 1: wouldn't it be the version from git-info/AllRepoVersions.props? But, I understand the issue with arcade-powered source-build and agree that option 1 isn't a good long-term approach.

dagood · 2020-11-13T19:31:04Z

I'm assuming the direct dependency would only be added in the offline build?

It can be if it causes problems in Production (or if it makes the patch clearer?), but I'd expect it to be fine to always have the direct dep. I already have a patch ready without this condition.

Regarding how to find the version for Option 1: wouldn't it be the version from git-info/AllRepoVersions.props?

That only has one value for the whole dotnet/runtime repo based on the package we have our Dependency indexed on, right? AFAIK we can't rely on that to be the right version of every single nupkg the repo produces--we'd have to add a Dependency for each package version we care about.

dseefeld · 2020-11-13T19:37:35Z

I'm assuming the direct dependency would only be added in the offline build?

It can be if it causes problems in Production (or if it makes the patch clearer?), but I'd expect it to be fine to always have the direct dep. I already have a patch ready without this condition.

Won't it be an issue for Arcade-powered source-build because that version won't exist?

Regarding how to find the version for Option 1: wouldn't it be the version from git-info/AllRepoVersions.props?

That only has one value for the whole dotnet/runtime repo based on the package we have our Dependency indexed on, right? AFAIK we can't rely on that to be the right version of every single nupkg the repo produces--we'd have to add a Dependency for each package version we care about.

Makes sense.

dagood · 2020-11-13T19:40:24Z

Won't it be an issue for Arcade-powered source-build because that version won't exist?

It's just set up for PVP--it'll use the version in eng/Version.props in the Production build. (dotnet/arcade actually already has a default version in eng/Version.props for these packages, it just isn't hooked up because it's not necessary in the Msft build.)

dagood · 2020-11-13T20:14:30Z

Microsoft.NETCore.Platforms/2.1.2 is not in a project.assets.json and I haven't looked into it yet:

The result I got earlier seems to be a red herring (it's just a layout copy from .dotnet/sdk/... intended to be used for testing, not actually indicating usage), and expanding my search a bit I still find nothing that leads to any actual usages:

#> grep -i 'Microsoft.NETCore.Platforms.*2.1.2' -r src/
src/runtime.cf258a14b70ad9069470a108f13765e0e5988f51/src/libraries/System.Text.Json/tests/Resources/Strings.resx:    <value>{"locked":false,"version":1,"targets":{"DNXCore,Version=v5.0":{"Mi...
src/runtime.cf258a14b70ad9069470a108f13765e0e5988f51/src/libraries/System.Text.Json/tests/Resources/Strings.resx:    <value>{  "locked": false,  "version": 1,  "targets": {    "DNXCore,Versi...
src/sdk.0cdbf80b4176ba4b979bdfc7b630b884f656072f/artifacts/bin/Release/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json:      "Microsoft.NETCore.Platforms/2.1.2": {},
src/sdk.0cdbf80b4176ba4b979bdfc7b630b884f656072f/artifacts/bin/Release/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json:          "Microsoft.NETCore.Platforms": "2.1.2",
src/sdk.0cdbf80b4176ba4b979bdfc7b630b884f656072f/artifacts/bin/Release/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json:    "Microsoft.NETCore.Platforms/2.1.2": {
src/sdk.0cdbf80b4176ba4b979bdfc7b630b884f656072f/artifacts/bin/Release/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json:      "path": "microsoft.netcore.platforms/2.1.2",
src/sdk.0cdbf80b4176ba4b979bdfc7b630b884f656072f/artifacts/bin/Release/Sdks/Microsoft.NET.Sdk.Razor/tools/rzc.deps.json:      "hashPath": "microsoft.netcore.platforms.2.1.2.nupkg.sha512"
src/arcade.6eec4404c2df5bfa46e5da52383c881c5cca3a9f/artifacts/obj/Microsoft.DotNet.GenFacades/project.assets.json:          "Microsoft.NETCore.Platforms": "2.1.2",

Let's keep the prebuilt for now and resolve it later by putting it in SBRP.

dagood · 2020-11-13T23:14:01Z

Remaining prebuilt that CI hit:

<AnnotatedUsage Id="System.Collections.Immutable" Version="5.0.0-preview.8.20407.11" SourceBuildPackageIdCreator="runtime SystemCollectionsImmutablePackageVersion/5.0.0" />

It looks like there's a usage without a project.assets.json but it was hiding behind another usage that did have a project.assets.json. 😛 Looking into it. At least the other removals worked.

dagood · 2020-11-16T16:11:54Z

Hmm, that's an interesting one. Suggests a type of usage I don't think we've considered before. It appears that dotnet/arcade's Microsoft.DotNet.GenFacades.csproj eventually decides to use System.Collections.Immutable/5.0.0, however the project has no direct dependency on it. At some point it extracts the prebuilt into the package cache for some reason.

"projectFileDependencyGroups": {
  ".NETCoreApp,Version=v2.1": [
    "Microsoft.Build >= 16.8.0",
    "Microsoft.Build.Tasks.Core >= 16.8.0",
    "Microsoft.CodeAnalysis.CSharp >= 3.8.0",
    "Microsoft.NETCore.App >= 2.1.0",
    "Microsoft.SourceLink.AzureRepos.Git >= 1.0.0",
    "Microsoft.SourceLink.GitHub >= 1.0.0",
    "System.Reflection.Metadata >= 5.0.0"
  ]
}

Msft.Build, Msft.Build.Tasks.Core, Msft.Build.Utilities.Core depend on System.Collections.Immutable >=1.5.0
Microsoft.CodeAnalysis.Common/3.8.0 depends on System.Collections.Immutable >=5.0.0-preview.8.20407.11
System.Reflection.Metadata/5.0.0 depends on System.Collections.Immutable >=5.0.0

When I delete Immutable as a prebuilt nupkg and build a tarball offline, I get this:

  "logs": [
    {
      "code": "NU1603",
      "level": "Error",
      "message": "Microsoft.CodeAnalysis.Common 3.8.0 depends on System.Collections.Immutable (>= 5.0.0-preview.8.20407.11) but System.Collections.Immutable 5.0.0-preview.8.20407.11 was not found. An approximate best match of System.Collections.Immutable 5.0.0 was resolved.",
      "libraryId": "System.Collections.Immutable",
      "targetGraphs": [
        ".NETCoreApp,Version=v2.1"
      ]
    }
  ]

So I'm guessing that it wants to try restoring both System.Reflection.Metadata/5.0.0 and /5.0.0-preview.8.20407.11 before it makes the choice to use 5.0.0. I'd guess this is related to the fact they're cousin dependencies in this project. I expect overriding with a top-level reference to 5.0.0 will work.

Microsoft.CodeAnalysis.CSharp brings in System.Collections.Immutable and System.Reflection.Metadata dependencies with prebuilt versions. We can override them to the version that we built. dotnet/arcade already defines SystemCollectionsImmutableVersion and SystemReflectionMetadataVersion, and it seems safe to use their values. See dotnet/source-build#1881

* Initial source-build config * Exclude unneeded project. * Reassign MsbuildTaskMicrosoftCodeAnalysisCSharpVersion in offline build Arcade has a special version prop for CodeAnalysis.CSharp in GenFacades to try to match the version loaded by msbuild. In the offline build, this is simply the source-built version. * Disable ILRewrite step - this isn't built in source-build so it fails. * Override Microsoft.CodeAnalysis.CSharp pkg deps Microsoft.CodeAnalysis.CSharp brings in System.Collections.Immutable and System.Reflection.Metadata dependencies with prebuilt versions. We can override them to the version that we built. dotnet/arcade already defines SystemCollectionsImmutableVersion and SystemReflectionMetadataVersion, and it seems safe to use their values. See dotnet/source-build#1881 Co-authored-by: Chris Rummel <crummel@microsoft.com> Co-authored-by: dseefeld <dseefeld@microsoft.com>

…et#6837) * Initial source-build config * Exclude unneeded project. * Reassign MsbuildTaskMicrosoftCodeAnalysisCSharpVersion in offline build Arcade has a special version prop for CodeAnalysis.CSharp in GenFacades to try to match the version loaded by msbuild. In the offline build, this is simply the source-built version. * Disable ILRewrite step - this isn't built in source-build so it fails. * Override Microsoft.CodeAnalysis.CSharp pkg deps Microsoft.CodeAnalysis.CSharp brings in System.Collections.Immutable and System.Reflection.Metadata dependencies with prebuilt versions. We can override them to the version that we built. dotnet/arcade already defines SystemCollectionsImmutableVersion and SystemReflectionMetadataVersion, and it seems safe to use their values. See dotnet/source-build#1881 Co-authored-by: Chris Rummel <crummel@microsoft.com> Co-authored-by: dseefeld <dseefeld@microsoft.com>

dagood self-assigned this Nov 12, 2020

dagood added 2 commits November 13, 2020 13:32

For 5.0.0, use bootstrapping in CI, not N-1

eff3ec3

Override Microsoft.CodeAnalysis.CSharp pkg deps

5cb50cf

dagood force-pushed the bootstrapping-ish-prev-sb branch from 7c2a93c to 5cb50cf Compare November 13, 2020 19:33

Update baseline (speculative: fix + merge)

057db59

dagood requested review from crummel and dseefeld November 13, 2020 20:19

crummel approved these changes Nov 13, 2020

View reviewed changes

Patch Microsoft.DotNet.GenFacades.csproj also

793c691

dagood merged commit e3a0069 into dotnet:master Nov 16, 2020

dagood deleted the bootstrapping-ish-prev-sb branch November 16, 2020 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For 5.0.0, use bootstrapping in CI, not N-1#1881

For 5.0.0, use bootstrapping in CI, not N-1#1881
dagood merged 4 commits intodotnet:masterfrom
dagood:bootstrapping-ish-prev-sb

dagood commented Nov 12, 2020

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dseefeld commented Nov 13, 2020

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dseefeld commented Nov 13, 2020 •

edited by dagood

Loading

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dagood commented Nov 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dagood commented Nov 12, 2020

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dseefeld commented Nov 13, 2020

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dseefeld commented Nov 13, 2020 • edited by dagood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dagood commented Nov 13, 2020

Uh oh!

dagood commented Nov 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dseefeld commented Nov 13, 2020 •

edited by dagood

Loading