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

Get release/3.1 to strict coherency #25050

Closed
19 tasks done
dougbu opened this issue Aug 19, 2020 · 20 comments
Closed
19 tasks done

Get release/3.1 to strict coherency #25050

dougbu opened this issue Aug 19, 2020 · 20 comments
Assignees
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Done This issue has been fixed
Milestone

Comments

@dougbu
Copy link
Member

dougbu commented Aug 19, 2020

Continue work started with https://github.com/dotnet/aspnetcore-internal/issues/3662#issuecomment-674301364

Working through some coherency issues in 'release/3.1' dependency PRs. See dotnet/razor#2376 (comment) for example. The issue is strict coherency (see https://github.com/mmitche/arcade/blob/7d66175e9e8e1a42e8448452777b524060bfb65e/Documentation/Darc.md#strict-coherency for documentation) requires intermediate repos to have everything downstream repos need when those downstream repos use CoherentParentDependency. Without that, the incremental servicing done in dotnet/corefx and dotnet/core-setup confuses things in our repos. We use CoherentParentDependency extensively due to "diamond dependency" issues when choosing between package versions flowing from dotnet/core-setup through dotnet/extensions then both dotnet/aspnetcore-tooling and dotnet/efcore into dotnet/aspnetcore.

As dependencies flow through, will need to do the same thing in dotnet/efcore for dotnet/aspnetcore requirements. dotnet/efcore lacks dependencies for most of what dotnet/extensions produces.

The command to see what's missing for strict coherency is

darc update-dependencies --strict-coherency --coherency-only

The command to double-check the URIs and SHAs in eng/Version.Details.xml is

[xml]$XmlDocument = Get-Content -Path .\eng\Version.Details.xml;$XmlDocument.Dependencies.ProductDependencies.Dependency | % { darc get-build --repo $_.Uri --commit $_.Sha | Out-Null ; if (-not $?) { Write-Host "$($_.Uri) @ $($_.Sha)" } }

Relevant PRs so far are:

Clean release/3.1 dependencies in our repos:

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

@mmitche @wtgodbe I see dotnet/corefx changes started flowing this morning (with dotnet/core-setup#9071). Does this mean we don't have time to resolve the rest of the strict coherency issues in 3.1.8❔ (URIs and SHAs are fine on #25048 branch but dotnet/core-setup lacks a dependency for System.Net.Http.Json and dotnet/efcore lacks many dependencies for dotnet/extensions packages.) I'd really like to at least do another PR in dotnet/efcore to address the second part of this. And, it would be great to know why we aren't seeing complaints about missed dotnet/extensions dependencies used in aspnetcore e.g. packages mentioned in https://github.com/dotnet/extensions/blob/bb7e14db3ce323db584de00434d8e7d5b06fae5c/eng/Versions.props#L88-L92
are a small subset of those in

<!-- For the targeting pack, always use packages with PatchVersion=0 -->
<PropertyGroup Condition="'$(MSBuildProjectName)' == 'Microsoft.AspNetCore.App.Ref' OR '$(IsReferenceAssemblyProject)' == 'true' ">
<SystemIOPipelinesPackageVersion>$(SystemIOPipelinesPackageVersion.Split('.')[0]).$(SystemIOPipelinesPackageVersion.Split('.')[1]).0</SystemIOPipelinesPackageVersion>
<SystemSecurityCryptographyXmlPackageVersion>$(SystemSecurityCryptographyXmlPackageVersion.Split('.')[0]).$(SystemSecurityCryptographyXmlPackageVersion.Split('.')[1]).0</SystemSecurityCryptographyXmlPackageVersion>
<MicrosoftWin32SystemEventsPackageVersion>$(MicrosoftWin32SystemEventsPackageVersion.Split('.')[0]).$(MicrosoftWin32SystemEventsPackageVersion.Split('.')[1]).0</MicrosoftWin32SystemEventsPackageVersion>
<SystemDiagnosticsEventLogPackageVersion>$(SystemDiagnosticsEventLogPackageVersion.Split('.')[0]).$(SystemDiagnosticsEventLogPackageVersion.Split('.')[1]).0</SystemDiagnosticsEventLogPackageVersion>
<SystemDrawingCommonPackageVersion>$(SystemDrawingCommonPackageVersion.Split('.')[0]).$(SystemDrawingCommonPackageVersion.Split('.')[1]).0</SystemDrawingCommonPackageVersion>
<SystemSecurityCryptographyPkcsPackageVersion>$(SystemSecurityCryptographyPkcsPackageVersion.Split('.')[0]).$(SystemSecurityCryptographyPkcsPackageVersion.Split('.')[1]).0</SystemSecurityCryptographyPkcsPackageVersion>
<SystemSecurityPermissionsPackageVersion>$(SystemSecurityPermissionsPackageVersion.Split('.')[0]).$(SystemSecurityPermissionsPackageVersion.Split('.')[1]).0</SystemSecurityPermissionsPackageVersion>
<SystemWindowsExtensionsPackageVersion>$(SystemWindowsExtensionsPackageVersion.Split('.')[0]).$(SystemWindowsExtensionsPackageVersion.Split('.')[1]).0</SystemWindowsExtensionsPackageVersion>
</PropertyGroup>

/cc @dotnet/aspnet-build
/fyi @BrennanConroy

@wtgodbe
Copy link
Member

wtgodbe commented Aug 19, 2020

@dougbu there's still time for 3.1.8 if we get PRs in today - we're waiting on a wpf-int change still, which can't go in until they get CTI validation on it tonight. Let me know if you'd like some help whipping up the remaining PRs.

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

Excellent❕

I'll take the big dotnet/efcore one. Would appreciate you taking adding System.Net.Http.Json to core-setup and making sure extensions knows about it. Can I add that dep to efcore before your change flows into that repo w/o breaking anything❔

@mmitche
Copy link
Member

mmitche commented Aug 19, 2020

I'd like to have strict coherency on by default by...say RC2 release or so. It involves a bunch of iterative changes so it's something I don't expect to get done instantly. Thanks for your help with this.

@wtgodbe
Copy link
Member

wtgodbe commented Aug 19, 2020

Would appreciate you taking adding System.Net.Http.Json to core-setup and making sure extensions knows about it.

dotnet/core-setup#9072

Can I add that dep to efcore before your change flows into that repo w/o breaking anything

Yep, that should be fine - it'd just be another non-strict dependency that'll get fixed once my change flows

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

I'd like to have strict coherency on by default by...say RC2 release or so.

This is about 3.1 changes which would be great to land in 3.1.8 so we don't need to keep pinning and unpinning random packages.

dougbu added a commit to dotnet/efcore that referenced this issue Aug 19, 2020
…cies

- dotnet/aspnetcore#25050
- for strict coherency in dotnet/aspnetcore

nit: move a few `System.*` dependencies down to make scanning file easier
@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

Shoot, I accidentally pushed into the 'release/3.1' branch. Could someone please check dotnet/efcore@8dfe3f7 for sanity❔ If it looks fine, I'll only revert the change if the official build fails.

@wtgodbe
Copy link
Member

wtgodbe commented Aug 19, 2020

@dougbu the versions themselves look good, but now the corefx & extensions dependencies are all mixed together

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

The dependencies were mixed before. I only made that a bit worse while improving the ability to scan alphabetically. It was the minimal change I considered.

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

I think we have only two smaller bits to figure out once the following changes / builds flow

  1. it would be great to know why we aren't seeing complaints about missed dotnet/extensions dependencies used in aspnetcore
  2. need a System.Net.Http.Json dependency in dotnet/extensions

@wtgodbe
Copy link
Member

wtgodbe commented Aug 19, 2020

it would be great to know why we aren't seeing complaints about missed dotnet/extensions dependencies used in aspnetcore

What exactly do you mean by this? Where do you expect to see complaints?

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

Where do you expect to see complaints?

In the darc update-dependencies --strict-coherency --coherency-only output when in the dotnet/aspnetcore repo. One sec while I confirm whether those versions are all auto-updated…

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

dotnet/aspnetcore contains

    <Dependency Name="Microsoft.Win32.SystemEvents" Version="4.7.0" CoherentParentDependency="Microsoft.NETCore.App.Runtime.win-x64">
      <Uri>https://github.com/dotnet/corefx</Uri>
      <Sha>0f7f38c4fd323b26da10cce95f857f77f0f09b48</Sha>
    </Dependency>
    <Dependency Name="System.Drawing.Common" Version="4.7.0" CoherentParentDependency="Microsoft.NETCore.App.Runtime.win-x64">
      <Uri>https://github.com/dotnet/corefx</Uri>
      <Sha>0f7f38c4fd323b26da10cce95f857f77f0f09b48</Sha>
    </Dependency>
    <Dependency Name="System.Security.Cryptography.Pkcs" Version="4.7.0" CoherentParentDependency="Microsoft.NETCore.App.Runtime.win-x64">
      <Uri>https://github.com/dotnet/corefx</Uri>
      <Sha>0f7f38c4fd323b26da10cce95f857f77f0f09b48</Sha>
    </Dependency>
    <Dependency Name="System.Security.Permissions" Version="4.7.0" CoherentParentDependency="Microsoft.NETCore.App.Runtime.win-x64">
      <Uri>https://github.com/dotnet/corefx</Uri>
      <Sha>0f7f38c4fd323b26da10cce95f857f77f0f09b48</Sha>
    </Dependency>
    <Dependency Name="System.Windows.Extensions" Version="4.7.0" CoherentParentDependency="Microsoft.NETCore.App.Runtime.win-x64">
      <Uri>https://github.com/dotnet/corefx</Uri>
      <Sha>0f7f38c4fd323b26da10cce95f857f77f0f09b48</Sha>
    </Dependency>

But, though the packages are the latest on NuGet, the URIs and SHAs are correct, the dependencies exist in dotnet/core-setup, and they're coherency dependencies in dotnet/aspnetcore, the above command doesn't complain about these packages. Why❔

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

BTW those dependencies don't exist in either dotnet/extensions or dotnet/efcore.

@dougbu
Copy link
Member Author

dougbu commented Aug 19, 2020

One thought: Is System.Net.Text.Json actually needed in dotnet/extensions and dotnet/efcore to satisfy dotnet/aspnetcore -- as long as it's in dotnet/core-setup❔ That is, are the only fixes needed where darc update-dependencies --strict-coherency --coherency-only states i.e. where the repo / branch where the coherency parent is produced❔

If that's the case, we might be done once https://dev.azure.com/dnceng/internal/_build/results?buildId=778882 (the build for dotnet/core-setup#9072) completes and the new versions flow to dotnet/aspnetcore.

@javiercn javiercn added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 20, 2020
@dougbu
Copy link
Member Author

dougbu commented Aug 20, 2020

I tested darc update-dependencies --source-repo core-setup --channel '.NET Core 3.1 Release' in the dotnet/ef6 repo and was surprised it did not bump up the System.Data.SqlClient version from 4.8.1 to 4.8.2. I had to run darc update-dependencies --strict-coherency --coherency-only to get that coherency update. Is that expected❔

Good news is that command found nothing else wrong.

Since we don't plan to build dotnet/ef6 for realz any time soon, I'm not going to push these changes or trigger the disabled core-setup subscription. Or, @bricelam would you prefer I bring ef6 up-to-date❔

@bricelam
Copy link
Contributor

No need to update ef6 for now.

@dougbu
Copy link
Member Author

dougbu commented Aug 20, 2020

And, … we're done. I've double-checked the 'release/3.1' branches in ASP.NET repos and see no strict coherency issues nor invalid URI / SHA combinations.

Still a bit surprised we're done because intermediate repos don't seem to need dependencies specified in incoming repos i.e. it seems fine dotnet/extensions doesn't list all corefx dependencies dotnet/aspnetcore requires.

@dougbu dougbu closed this as completed Aug 20, 2020
@dougbu dougbu added this to the 3.1.8 milestone Aug 20, 2020
@dougbu dougbu added the Done This issue has been fixed label Aug 20, 2020
@mmitche
Copy link
Member

mmitche commented Aug 20, 2020

Still a bit surprised we're done because intermediate repos don't seem to need dependencies specified in incoming repos i.e. it seems fine dotnet/extensions doesn't list all corefx dependencies dotnet/aspnetcore requires.

It's because aspnetcore ties the corefx dependencies to Microsoft.NETCore.App.Runtime.win-x64, not extensions. So core-setup needs to list them all, but not extensions. Extensions only needs to list Microsoft.NETCore.App.Runtime.win-x64

@dougbu
Copy link
Member Author

dougbu commented Aug 20, 2020

Thanks for the confirmation @mmitche❕ As I mentioned earlier, I thought something like that was going on.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

5 participants