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

Updates to enable PVP flow #13009

Closed
wants to merge 2 commits into from
Closed

Updates to enable PVP flow #13009

wants to merge 2 commits into from

Conversation

mthalman
Copy link
Member

@mthalman mthalman commented Apr 3, 2023

In order to enable PVP flow for the arcade repo, it's necessary to remove some workarounds that were added to explicitly bump up the referenced version of the System.Collections.Immutable and System.Reflection.Metadata packages for source-build since it uses a higher version in that build than it does in the arcade repo. PVP flow now allows the package versions defined by the repo to be used directly.

It was also necessary to add the following two packages to Version.Details.xml:

  • Microsoft.Extensions.DependencyModel.6.0.0
  • System.IO.Packaging.4.5.0

Defining them here enables them, with PVP flow, to be retrieved from previously-source-built artifacts instead of SBRP. Both of these packages are needed at build-time; they are literally executed during the build. Since they need to be executed, retrieving the package from SBRP is not desired since that only contains ref assemblies.

In the case of System.IO.Packaging, it is a dependency of Microsoft.DotNet.NuGetRepack.Tasks. But since the System.IO.Packaging.dll is a ref assembly, it doesn't get included in the lib directory of the Microsoft.DotNet.NuGetRepack.Tasks package. Then downstream, the symreader repo had a dependency on Microsoft.DotNet.NuGetRepack.Tasks and when attempting to load Microsoft.DotNet.NuGetRepack.Tasks.dll, it failed because System.IO.Packaging.dll did not exist. Using the non-ref assembly from previously-source-built artifacts solves this issue.

Similarly, Microsoft.Extensions.DependencyModel ends up being used during the build and fails in the build of the runtime repo, as described by dotnet/runtime#84925, because it's attempting to execute a ref assembly.

Related: #6837, #6971

Contributes to dotnet/source-build#3043

@mthalman mthalman changed the title Remove SB workarounds to enable PVP flow Updates to enable PVP flow Apr 18, 2023
@mthalman mthalman marked this pull request as ready for review April 18, 2023 18:01
@mthalman
Copy link
Member Author

We need to wait on merging this PR because it requires coordinating changes in installer (source-build) first. Let's iterate on the changes here in this PR. Then I'll create patches for it in installer along with enabling PVP flow. Then it will be ok for this PR to be merged. I guess for now, I'll mark this as draft so it doesn't get accidentally merged. But it is ready for review.

@mthalman mthalman marked this pull request as draft April 18, 2023 18:04
@mthalman mthalman requested a review from a team April 18, 2023 18:08
@@ -95,6 +95,12 @@
<Uri>https://github.com/dotnet/deployment-tools</Uri>
<Sha>b60c95e1ce736630d17e16626c59e3dd85ebae2b</Sha>
</Dependency>
<!-- Necessary for source-build. This allows the package to be retrieved from previously-source-built artifacts
and flow in as dependencies of the packages produced by arcade. -->
<Dependency Name="Microsoft.Extensions.DependencyModel" Version="6.0.0">
Copy link
Member

Choose a reason for hiding this comment

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

IMO, both of these new dependencies should be live versions otherwise there is a significant disconnect between the repo level source-build and the production source-build.

Copy link
Member Author

Choose a reason for hiding this comment

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

How can they be live dependencies when arcade gets built first? Or am I misunderstanding what is meant by live dependencies?

Copy link
Member

Choose a reason for hiding this comment

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

By live dependency I mean there is a darc subscription to update it on a specific cadence to latest. In source-build it will pick up the version from previously-source-built.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's the right approach unless we think that the surface area is changing in such a way that it is likely to break when built against latest. This repo isn't taking advantage of new surface area.

Copy link
Member

Choose a reason for hiding this comment

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

So I can better understand your viewpoint @mmitche, does .NET 9 change your viewpoint at all?

Copy link
Member

Choose a reason for hiding this comment

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

Not at first glance. Let me think on it.

Copy link
Member

Choose a reason for hiding this comment

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

I was just talking with @oleksandr-didyk and remembered why 6.0.0 (an SBRP) won't work for the repo source-build scenario. The Arcade components that reference Microsoft.Extensions.DependencyModel are loaded/used in downstream repos. When an SBRP is referenced an exception will be thrown. @oleksandr-didyk discovered this earlier in the runtime source-build leg. @mthalman discovered this in the product source-build which is why he is declaring this verion.details dependency is needed.

@oleksandr-didyk
Copy link
Contributor

oleksandr-didyk commented Jun 2, 2023

Closing as this PR is stale + its goals were achieved in #13754 and dotnet/source-build-reference-packages#691

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants