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

Fix empty combobox when package is not present in project file #4844

Merged
merged 3 commits into from
Oct 6, 2022

Conversation

martinrrm
Copy link
Contributor

Bug

Fixes: NuGet/Home#12057

Regression? Last working version:

Description

For some type of projects there are installed packages that are not listed in the project file (.csproj). This was not considered when developing the combobox version and breaks when trying to get the OriginalVersion.

Before:
image

After:
image

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

@martinrrm martinrrm requested a review from a team as a code owner October 5, 2022 07:50
jeffkl
jeffkl previously approved these changes Oct 5, 2022
erdembayar
erdembayar previously approved these changes Oct 5, 2022
@@ -167,7 +167,7 @@ protected override Task CreateVersionsAsync(CancellationToken cancellationToken)
var latestStableVersion = allVersionsAllowed.FirstOrDefault(v => !v.version.IsPrerelease);

// Add installed version if the project is PackageReference
if (_nugetProjects.Any() && installedDependency != null && installedDependency.VersionRange != null && _nugetProjects.First().ProjectStyle.Equals(ProjectModel.ProjectStyle.PackageReference))
if (_nugetProjects.Any() && installedDependency != null && installedDependency.VersionRange.OriginalString != null && _nugetProjects.First().ProjectStyle.Equals(ProjectModel.ProjectStyle.PackageReference))
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to understand why original string would be null when a package is not in the csproj file.

Is the package not being in the csproj file the problem, or is it the fact that it's autoreferenced?

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'm not sure if it is because is auto referenced but it could also be the problem. I was taking the .csproj as the only source for packages. When creating the versions from the project file we create the object from a string using VersionRange.Parse(string), this is the only way the attribute OriginalString is populated, so I think because this package is autoreferenced, im not sure how but probably using new VersionRange(Version)

Copy link
Member

Choose a reason for hiding this comment

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

I'd find it surprising if we're generating the Version Range differently for autoreferenced package.

At least in the nomination workflow. Maybe there's something we do differently in the PM UI side?

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 double check this behavior

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 found that when the package is autoreferenced we use new VersionRange() and this doesnt populates the OriginalString attribute. https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.PackageManagement/BuildIntegratedPackageReference.cs#L91

Copy link
Member

@nkolev92 nkolev92 Oct 5, 2022

Choose a reason for hiding this comment

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

I see.
A few ideas:

  • We can consider making sure that we have an original version there no? Just doing that is probably not the right fix, but it's a valid consideration.
  • If OriginalString isn't an option should we show the normalized version instead?

Copy link
Member

Choose a reason for hiding this comment

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

Please note that for an auto referenced version you should not be able to update the version through the PM UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • OriginalString cannot be set, only by using VersionRange.Parse()
  • The change in L170 is not affecting functionality, right we are displaying the version that the user wrote in the .csproj at the top and then all the versions, with this fix, if the OriginalString is null then we will display the latest version and then all the versions, like before but with filtering

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 tried considering using the normalized version when the OriginalString was null, but this created more issues when adding the blocking versions later in the code and would need more changes.

@martinrrm martinrrm dismissed stale reviews from erdembayar and jeffkl via d0a0ab0 October 6, 2022 00:23
@@ -842,7 +842,7 @@ protected void AddBlockedVersions(List<NuGetVersion> blockedVersions)
// add all the versions blocked to disable the update button
foreach (var version in blockedVersions)
{
_versions.Add(new DisplayVersion(version, string.Empty, isValidVersion: false));
_versions.Add(new DisplayVersion(version, additionalInfo: null, isValidVersion: false));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -326,7 +327,7 @@ public bool IsInstallorUpdateButtonEnabled
{
get
{
return SelectedVersion != null && !IsSelectedVersionInstalled;
return SelectedVersion != null && !IsSelectedVersionInstalled && !InstalledVersionIsAutoReferenced;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the package is autoreferenced then block the Update button

@nkolev92
Copy link
Member

nkolev92 commented Oct 6, 2022

Manually debugging the fix appears to work.
There are some null ref exceptions being raised. I'll investigate whether the fix has anything to do with that.

@nkolev92
Copy link
Member

nkolev92 commented Oct 6, 2022

Initial look it doesn't seem related. I'll merge when the build passes.

@nkolev92 nkolev92 merged commit d8a2220 into dev Oct 6, 2022
@nkolev92 nkolev92 deleted the dev-martinrrm-fix-combobox-package-not-in-projectfile branch October 6, 2022 16:47
nkolev92 added a commit that referenced this pull request Oct 7, 2022
#4848)

Co-authored-by: Martin Ruiz <martin.ruiz.mares@gmail.com>
AdmiringWorm added a commit to chocolatey/NuGet.Client that referenced this pull request Dec 19, 2022
Insert 6.4.0-rc.123 into rel/d17.4 on 11/07/2022 23:47:12

* tag '6.4.0.123': (60 commits)
  fix a logic error that caused AbandonedMutexException while executing migrations (release-6.4.x) (NuGet#4895)
  unblock source build failing due to fatal: transport 'file' not allowed error (NuGet#4867) (NuGet#4874)
  Signing:  update to August 2022 CTL (NuGet#4791) (NuGet#4850)
  Merged PR 422933: Prefer BCL Directory create API over helper class (7.0.1xx-rc2)
  Fix empty combobox when package is not present in project file (NuGet#4844) (NuGet#4848)
  Fix component detection alert for microsoft.owin package (NuGet#4841) (NuGet#4845)
  Make release label RC, move to escrow mode
  Adds special case to include transitive origins in GetInstalledAndTransitivePackagesAsync API (NuGet#4824)
  Add longPathAware manifest to NuGet.Build.Tasks.Console (NuGet#4830)
  VsPackageInstallerServices should not post ProjectNotNominatedException faults (NuGet#4814)
  Skip test GetOrCreateAsync_WithUnhandledExceptionInPlugin_Throws (NuGet#4831)
  Improve OptProf pipeline job run names (NuGet#4825)
  Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio (NuGet#4798)
  Suppress CA2213 warnings to unblock dev branch (NuGet#4823)
  Ensure IsVsOfflineFeed is calculated correctly on 64-bit machines (NuGet#4817)
  Add better handling of AggregateExceptions in static graph-based restore (NuGet#4809)
  Add Component Detection task into each pipeline (NuGet#4813)
  Localizes nuget.exe with default, embedded resource assembly lookup (NuGet#4773)
  Removes BrowseObjectBase class in NuGet Solution Explorer (NuGet#4807)
  Improve TryCreateContext  (NuGet#4762)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants