Skip to content

Set CPM metadata during preview restore when installing a package in Visual Studio#4642

Merged
jeffkl merged 3 commits intodevfrom
dev-jeffkl-fix-vs-install-package-with-cpm
Jun 27, 2022
Merged

Set CPM metadata during preview restore when installing a package in Visual Studio#4642
jeffkl merged 3 commits intodevfrom
dev-jeffkl-fix-vs-install-package-with-cpm

Conversation

@jeffkl
Copy link
Contributor

@jeffkl jeffkl commented May 24, 2022

Bug

Fixes: NuGet/Home#11828

Regression? Last working version:

Description

This code is used to modify the in-memory representation of the package spec and is modified to do a preview restore before committing changes via project system. The code needs to set the VersionCentrallyManaged property if central package management is enabled otherwise the restore fails since it thinks the user set the version in a project.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A

  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@jeffkl jeffkl force-pushed the dev-jeffkl-fix-vs-install-package-with-cpm branch from 0dee0fc to e344062 Compare May 27, 2022 08:09
@jeffkl jeffkl changed the title Set VersionCentrallyManaged when installing a package in Visual Studio Set CPM metadata during preview restore when installing a package in Visual Studio May 27, 2022
@jeffkl jeffkl marked this pull request as ready for review May 27, 2022 08:12
@jeffkl jeffkl requested a review from a team as a code owner May 27, 2022 08:12
@jeffkl
Copy link
Contributor Author

jeffkl commented May 27, 2022

FYI @tmeschter this is the change needed to get VS to actually install the package with CPM enabled. Technically you can build this branch and deploy it to your local experimental instance if you want to also play around with changes on your end. If you're interested in trying that and have problems, let me know.

nkolev92
nkolev92 reviewed May 27, 2022
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

I think we're missing one scenario.

@nkolev92
Copy link
Member

nkolev92 commented May 27, 2022

It might be a good idea to add cross tool no-op tests for these scenarios.
I've had the vendors run manual tests, and also some automated tests:

NuGet.Client/test/EndToEnd/tests/NetCoreProjectTest.ps1

Lines 165 to 187 in 0da9fda

function Test-NetCoreTargetFrameworksVSandMSBuildNoOp {
# Arrange
$project = New-NetCoreConsoleTargetFrameworksApp ConsoleApp
Build-Solution
Assert-ProjectCacheFileExists $project
$cacheFile = Get-ProjectCacheFilePath $project
#Act
$VSRestoreTimestamp =( [datetime](Get-ItemProperty -Path $cacheFile -Name LastWriteTime).lastwritetime).Ticks
$MSBuildExe = Get-MSBuildExe
& "$MSBuildExe" /t:restore $project.FullName
Assert-True ($LASTEXITCODE -eq 0)
$MsBuildRestoreTimestamp =( [datetime](Get-ItemProperty -Path $cacheFile -Name LastWriteTime).lastwritetime).Ticks
#Assert
Assert-True ($MsBuildRestoreTimestamp -eq $VSRestoreTimestamp)
}

@jeffkl
Copy link
Contributor Author

jeffkl commented May 31, 2022

It might be a good idea to add cross tool no-op tests for these scenarios.

I think I need to open a new item to add some end-to-end tests for CPM. The scenario to test here would be to install a new package and check if a restore immediately after no-ops. But there isn't any existing end-to-end tests that configure CPM as far as I can tell. Are you okay with me adding some stuff like this later? I did test the scenario manually and the restore no-ops.

nkolev92
nkolev92 reviewed Jun 7, 2022
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

The immediate change looks fine.
We might need to update the package spec of the parents if they are in the same context to prevent random failures at update time.

@jeffkl jeffkl self-assigned this Jun 14, 2022
@pragnya17 pragnya17 mentioned this pull request Jun 15, 2022
Closed
5 tasks
@kartheekp-ms kartheekp-ms mentioned this pull request Jun 16, 2022
Merged
This was referenced Jun 21, 2022
Closed
Open
@jeffkl
Copy link
Contributor Author

jeffkl commented Jun 23, 2022

I filed NuGet/Home#11912 for a future effort to see if we can include other affected projects in the install/update package so that after changes are committed, restore is a no-op.

@nkolev92 This is ready for final review

@jeffkl
Copy link
Contributor Author

jeffkl commented Jun 23, 2022

I've also filed an issue to work on end-to-end tests for CPM: NuGet/Home#11913

@pragnya17 pragnya17 mentioned this pull request Jun 23, 2022
Merged
5 tasks
@jeffkl
Copy link
Contributor Author

jeffkl commented Jun 24, 2022

@NuGet/nuget-client please review if you want, otherwise I'll merge this afternoon.

@jeffkl jeffkl merged commit c48ecf8 into dev Jun 27, 2022
@jeffkl jeffkl deleted the dev-jeffkl-fix-vs-install-package-with-cpm branch June 27, 2022 16:59
pragnya17 added a commit that referenced this pull request Jun 29, 2022
@pragnya17
Revert "Set CPM metadata during preview restore when installing a pac…
af76f07 
…kage in Visual Studio (#4642)"

This reverts commit c48ecf8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nkolev92 nkolev92 nkolev92 approved these changes

@zivkan zivkan zivkan approved these changes

Assignees

@jeffkl jeffkl

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Installing a package in Visual Studio when using central package management results in an error before projects can be modified

3 participants

@jeffkl @nkolev92 @zivkan