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

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

Merged
merged 3 commits into from
Jun 27, 2022

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.

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

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:

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.

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
@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

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants