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

Enable PVP flow for nuget.client #16603

Merged
merged 4 commits into from
Jun 8, 2023

Conversation

NikolaMilosavljevic
Copy link
Member

Contributes to dotnet/source-build#3043

Enables PVP flow for nuget.client repo. Includes a patch for the repo-side work: NuGet/NuGet.Client#5207

@NikolaMilosavljevic NikolaMilosavljevic requested a review from a team as a code owner June 6, 2023 21:06
Copy link
Member

@mthalman mthalman left a comment

Choose a reason for hiding this comment

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

I question whether we really gain much by enabling this now (which requires a patch) versus enabling it once the nuget change flows to installer. The friction here is that the existence of this patch will cause the build for the dependency flow with the nuget change to fail.

@NikolaMilosavljevic
Copy link
Member Author

I question whether we really gain much by enabling this now (which requires a patch) versus enabling it once the nuget change flows to installer. The friction here is that the existence of this patch will cause the build for the dependency flow with the nuget change to fail.

That is generally true for any patch. This one was requested today. cc @MichaelSimons

@mthalman
Copy link
Member

mthalman commented Jun 6, 2023

That is generally true for any patch. This one was requested today. cc @MichaelSimons

Yes, I know. Some patches are more important than others. I'm questioning whether this is that important.

@MichaelSimons
Copy link
Member

I question whether we really gain much by enabling this now (which requires a patch) versus enabling it once the nuget change flows to installer. The friction here is that the existence of this patch will cause the build for the dependency flow with the nuget change to fail.

That's a fair question. NuGet is very sporadic on when it flows in. It can take a long time for these changes to flow in. This is the reason I asked for the patch. The patch itself has a low risk for conflict so the only overhead is the dependency flow.

The benefit is that we can complete the pvp work and validate the complete e2e. Validating each repo independently is not enough.

@NikolaMilosavljevic
Copy link
Member Author

NikolaMilosavljevic commented Jun 6, 2023

9 source-build smoke-tests failed - likely unrelated as my local VMR build was clean, but with an older base sha.

@NikolaMilosavljevic
Copy link
Member Author

9 source-build smoke-tests failed - likely unrelated as my local VMR build was clean, but with an older base sha.

Hmm, this could be related as I don't see any failures in other CI runs. Likely a transitive dependency on a reference package. Investigating...

@NikolaMilosavljevic
Copy link
Member Author

Fixed the source-build issue with 8078c5a

<MicrosoftBuildVersion Condition="'$(TargetFramework)' == 'netcoreapp5.0'">16.11.0</MicrosoftBuildVersion>

<!-- Overridden by source build to ensure the same version is used across products, do not remove these properties -->
+ <!-- For each property here, there is an appropriate package dependency in eng/Version.Details.xml -->
Copy link
Member

Choose a reason for hiding this comment

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

In order to reduce the chance of patch conflicts, this change should be removed since it provides no functional behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, makes sense. I'll remove it from the patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed with 4604dec

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.

3 participants