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.

Sign up for GitHub

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

Disable floating versions in project level for CPM #5011

Merged
merged 6 commits into from
Feb 24, 2023
Merged

Disable floating versions in project level for CPM #5011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4a02821
disable install/update package button for CPM projects in PM UI
martinrrm Dec 13, 2022
47a4f24
pr comments
martinrrm Jan 12, 2023
eb0c859
disable button
martinrrm Jan 18, 2023
5b03989
pr comments
martinrrm Feb 7, 2023
3505a70
pr comments
martinrrm Feb 10, 2023
5aacf80
pr comments
martinrrm Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Models/DetailControlModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,8 @@ public OptionsViewModel Options

public string PackagePath => _searchResultPackage?.PackagePath;

public bool IsCentralPackageManagementEnabled { get; set; }
donnie-msft marked this conversation as resolved.
Show resolved Hide resolved

protected void AddBlockedVersions(List<NuGetVersion> blockedVersions)
{
// add a separator
Expand Down
8 changes: 8 additions & 0 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Models/PackageDetailControlModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,14 @@ public bool IsProjectPackageReference
}
}

public bool IsFloatingVersionSupported
{
get
{
return IsProjectPackageReference && !IsCentralPackageManagementEnabled;
martinrrm marked this conversation as resolved.
Show resolved Hide resolved
}
}

public bool IsInstalledVersionTopLevel => InstalledVersion != null && PackageLevel == PackageLevel.TopLevel;

public override IEnumerable<IProjectContextInfo> GetSelectedProjects(UserAction action)
Expand Down
15 changes: 15 additions & 0 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerControl.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ private async ValueTask InitializeAsync(PackageManagerModel model, INuGetUILogge
ApplySettings(settings, Settings);
_initialized = true;

await IsCentralPackageManagementEnabledAsync(CancellationToken.None);

NuGetExperimentationService = await ServiceLocator.GetComponentModelServiceAsync<INuGetExperimentationService>();
_isTransitiveDependenciesExperimentEnabled = NuGetExperimentationService.IsExperimentEnabled(ExperimentationConstants.TransitiveDependenciesInPMUI);

Expand Down Expand Up @@ -567,6 +569,19 @@ await RunAndEmitRefreshAsync(async () =>
}
}

private async Task IsCentralPackageManagementEnabledAsync(CancellationToken cancellationToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly then CPM is enabled for all projects or not, so just checking for 1st project would be suffice.

Suggested change
private async Task IsCentralPackageManagementEnabledAsync(CancellationToken cancellationToken)
private async Task IsCentralPackageManagementEnabledAsync(CancellationToken cancellationToken)
{
if (!Model.IsSolution)
{
await NuGetUIThreadHelper.JoinableTaskFactory.RunAsync(async delegate
{
// Go off the UI thread to perform non-UI operations
await TaskScheduler.Default;
if (Model.Context.Projects.Any())
{
_detailModel.IsCentralPackageManagementEnabled = await Model.Context.Projects.First().IsCentralPackageManagementEnabledAsync(Model.Context.ServiceBroker, cancellationToken);
}
});
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You can skip the Any() and just use the First() as I suggested here: #5011 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

But that case we're making assumption, there would be at least 1 project, Any is for safeguarding any unexpected null exception error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go with First() because as donnie said, we use it on other places and we're not afraid to make that assertion. I would normally go with Any() to avoid errors but the PM UI cannot be opened without a project

Copy link
Contributor

Choose a reason for hiding this comment

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

You still didn't implement First(). For you no need to loop. You can use above code without Any() check too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I forgot to push new changes.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly then CPM is enabled for all projects or not

That's not correct. CPM (and PackageReference) is implemented via MSBuild, and MSBuild is strictly per-project. Trying to enforce anything to be "all of solution or none" in MSBuild is effectively impossible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@martinrrm
Have you looked into above scenario? Do we consider it CPM enabled if some projects are CPM enabled but not all? Maybe check with @jeffkl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code will only run in the project PM UI, so if the user has disabled CPM for that project, it can install any version. I understand that disable CPM in just a project is not a desired behavior for this feature but since its msbuild it difficult to restrain users from doing it. @jeffkl can you confirm this?

{
if (!Model.IsSolution)
{
await NuGetUIThreadHelper.JoinableTaskFactory.RunAsync(async delegate
{
// Go off the UI thread to perform non-UI operations
await TaskScheduler.Default;
_detailModel.IsCentralPackageManagementEnabled = await Model.Context.Projects.First().IsCentralPackageManagementEnabledAsync(Model.Context.ServiceBroker, cancellationToken);
});
}
}

private async Task<string> GetSettingsKeyAsync(CancellationToken cancellationToken)
{
string key;
Expand Down
8 changes: 4 additions & 4 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/ProjectView.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,10 @@
MinHeight="22"
AutomationProperties.Name="{x:Static local:Resources.Label_Version}"
AutomationProperties.LabeledBy="{Binding ElementName=SubscriptionsLabel}"
IsEditable="{Binding IsProjectPackageReference, Converter={StaticResource InverseNullToVisibilityConverter}}"
IsTextSearchEnabled="{Binding IsProjectPackageReference, Converter={StaticResource NotNullOrTrueToBooleanConverter}}"
IsReadOnly="{Binding IsProjectPackageReference, Converter={StaticResource NotNullOrTrueToBooleanConverter}}"
StaysOpenOnEdit="{Binding IsProjectPackageReference, Converter={StaticResource InverseNullToVisibilityConverter}}"
IsEditable="{Binding IsFloatingVersionSupported, Converter={StaticResource InverseNullToVisibilityConverter}}"
IsTextSearchEnabled="{Binding IsFloatingVersionSupported, Converter={StaticResource NotNullOrTrueToBooleanConverter}}"
IsReadOnly="{Binding IsFloatingVersionSupported, Converter={StaticResource NotNullOrTrueToBooleanConverter}}"
StaysOpenOnEdit="{Binding IsFloatingVersionSupported, Converter={StaticResource InverseNullToVisibilityConverter}}"
ItemsSource="{Binding Path=VersionsView}"
Text="{Binding Path=UserInput, Mode=OneWay}"
SelectionChanged="Versions_SelectionChanged"
Expand Down
17 changes: 17 additions & 0 deletions ...uGet.Clients/NuGet.PackageManagement.VisualStudio/Common/IProjectContextInfoExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
using NuGet.Frameworks;
using NuGet.Packaging.Core;
using NuGet.VisualStudio.Internal.Contracts;
using NuGet.ProjectManagement.Projects;
using System.Linq;

namespace NuGet.PackageManagement.VisualStudio
{
Expand Down Expand Up @@ -123,6 +125,21 @@ public static async ValueTask<IReadOnlyCollection<NuGetFramework>> GetTargetFram
}
}

public static async ValueTask<bool> IsCentralPackageManagementEnabledAsync(this IProjectContextInfo projectContextInfo,
IServiceBroker serviceBroker,
CancellationToken cancellationToken)
{
Assumes.NotNull(projectContextInfo);
Assumes.NotNull(serviceBroker);

cancellationToken.ThrowIfCancellationRequested();

using (INuGetProjectManagerService projectManager = await GetProjectManagerAsync(serviceBroker, cancellationToken))
{
return await projectManager.IsCentralPackageManagementEnabledAsync(projectContextInfo.ProjectId, cancellationToken);
}
}

public static async ValueTask<IProjectMetadataContextInfo> GetMetadataAsync(
this IProjectContextInfo projectContextInfo,
IServiceBroker serviceBroker,
Expand Down
24 changes: 24 additions & 0 deletions ...NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/NuGetProjectManagerService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,30 @@ public async ValueTask<IReadOnlyCollection<NuGetFramework>> GetTargetFrameworksA
return targetFrameworks;
}

public async ValueTask<bool> IsCentralPackageManagementEnabledAsync(string projectId, CancellationToken cancellationToken)
martinrrm marked this conversation as resolved.
Show resolved Hide resolved
{
Assumes.NotNullOrEmpty(projectId);

cancellationToken.ThrowIfCancellationRequested();

NuGetProject? project = await SolutionUtility.GetNuGetProjectAsync(
_sharedState.SolutionManager,
projectId,
cancellationToken);

Assumes.NotNull(project);

if (project is BuildIntegratedNuGetProject buildIntegratedProject)
{
var dgcContext = new DependencyGraphCacheContext();
IReadOnlyList<ProjectModel.PackageSpec>? packageSpecs = await buildIntegratedProject.GetPackageSpecsAsync(dgcContext);

return packageSpecs.Any(spec => spec.RestoreMetadata.CentralPackageVersionsEnabled);
}

return false;
}

public async ValueTask<IProjectMetadataContextInfo> GetMetadataAsync(string projectId, CancellationToken cancellationToken)
{
Assumes.NotNullOrEmpty(projectId);
Expand Down
1 change: 1 addition & 0 deletions src/NuGet.Clients/NuGet.VisualStudio.Internal.Contracts/INuGetProjectManagerService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ ValueTask<IReadOnlyCollection<PackageDependencyInfo>> GetInstalledPackagesDepend
string projectId,
bool includeUnresolved,
CancellationToken cancellationToken);
ValueTask<bool> IsCentralPackageManagementEnabledAsync(string projectId, CancellationToken cancellationToken);
ValueTask<IProjectMetadataContextInfo> GetMetadataAsync(string projectId, CancellationToken cancellationToken);
ValueTask<IProjectContextInfo> GetProjectAsync(string projectId, CancellationToken cancellationToken);
ValueTask<IReadOnlyCollection<IProjectContextInfo>> GetProjectsAsync(CancellationToken cancellationToken);
Expand Down
1 change: 1 addition & 0 deletions src/NuGet.Clients/NuGet.VisualStudio.Internal.Contracts/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
#nullable enable
NuGet.VisualStudio.Internal.Contracts.INuGetProjectManagerService.IsCentralPackageManagementEnabledAsync(string! projectId, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask<bool>
48 changes: 48 additions & 0 deletions ...sts/NuGet.PackageManagement.VisualStudio.Test/Services/NuGetProjectManagerServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1526,6 +1526,54 @@ private async Task GetPackageFoldersAsync_LegacyProjectWithFallbackFolder_Return
Assert.Equal(2, folders.Count);
}

[Theory]
[InlineData(true)]
[InlineData(false)]
private async Task GetCentralPackageVersionsManagmentEnabled_SucceedsAsync(bool isCentralPackageVersionsEnabled)
{
string projectName = Guid.NewGuid().ToString();
string projectId = projectName;
var projectSystemCache = new ProjectSystemCache();
IVsProjectAdapter projectAdapter = Mock.Of<IVsProjectAdapter>();

using var pathContext = new SimpleTestPathContext();
Initialize();

// Prepare: Create project
string projectFullPath = Path.Combine(pathContext.SolutionRoot, projectName, $"{projectName}.csproj");

CpsPackageReferenceProject prProject = CreateCpsPackageReferenceProject(projectName, projectFullPath, projectSystemCache);

ProjectNames projectNames = GetTestProjectNames(projectFullPath, projectName);
string referenceSpec = $@"
{{
""frameworks"":
{{
""net6.0"":
{{
""dependencies"":
{{
}}
}}
}}
}}";
PackageSpec packageSpec = JsonPackageSpecReader.GetPackageSpec(referenceSpec, projectName, projectFullPath).WithTestRestoreMetadata();
packageSpec.RestoreMetadata.CentralPackageVersionsEnabled = isCentralPackageVersionsEnabled;

// Restore info
DependencyGraphSpec projectRestoreInfo = ProjectTestHelpers.GetDGSpecFromPackageSpecs(packageSpec);
projectSystemCache.AddProjectRestoreInfo(projectNames, projectRestoreInfo, new List<IAssetsLogMessage>());
projectSystemCache.AddProject(projectNames, projectAdapter, prProject).Should().BeTrue();

_solutionManager.NuGetProjects.Add(prProject);

// Act
bool isCentralPackageManagmentEnabled = await _projectManager.IsCentralPackageManagementEnabledAsync(projectId, CancellationToken.None);

// Assert
Assert.Equal(isCentralPackageVersionsEnabled, isCentralPackageManagmentEnabled);
}

[Fact]
private async Task GetPackageFoldersAsync_CpsProjectWithFallbackFolder_ReturnsPackageFoldersAsync()
{
Expand Down