Added support for better handling of transitive dependencies #539

popara96 · 2023-07-14T15:59:35Z

Details of new usage are explained in README file.

JoC0de

Hi @popara96,
thanks for the contribution. It was a much requested feature, I also wanted to include the feature but had no time. It generally looks good, I added some comments it would be nice if you fix them.
Also one UnitTest is failing.

JoC0de · 2023-07-16T18:31:51Z

.gitignore

@@ -1,3 +1,4 @@
 /[Bb]in/

 *.log
+/src/NuGetForUnity.CreateDll/.idea/**


Suggested change

/src/NuGetForUnity.CreateDll/.idea/**

**/.idea

JoC0de · 2023-07-16T18:37:15Z

src/NuGetForUnity/Editor/NugetHelper.cs

@@ -67,6 +67,8 @@ public static class NugetHelper
        /// </summary>
        private static readonly List<NugetPackageSource> packageSources = new List<NugetPackageSource>();

+        public static event Action OnInstalledPackagesChanged;


I don't like events especially static ones. The UI (NugetWindow) knows when packages are installed so it don't needs to depend on NugetHelper to manage a event.

I remember there being a code earlier that would automatically trigger Restore when packages.config file was changed and we thought we needed this event because of that case but it seems that code is now only triggered the first time project is loaded.

Is that right? Does that mean if I install some packages and push packages.config other developers won't automatically get that package installed when they pull my changes if they already have project open in Unity and they will have to manually run Restore?

Yes for packages.config change detection we would need to implement a AssetPostprocessor / add this feature to the NugetPackageAssetPostprocessor

JoC0de · 2023-07-16T18:46:38Z

src/NuGetForUnity/Editor/NugetHelper.cs

+            installedPackages?.Remove(foundPackage.Id);
+
+            if (installedPackages != null && installedPackages.Count > 0)
+            {


Suggested change

installedPackages?.Remove(foundPackage.Id);

if (installedPackages != null && installedPackages.Count > 0)

{

if (installedPackages != null && installedPackages.Count > 0)

{

installedPackages.Remove(foundPackage.Id);

JoC0de · 2023-07-16T19:07:01Z

src/NuGetForUnity/Editor/NugetHelper.cs

+
+            if (installedPackages != null && installedPackages.Count > 0)
+            {
+                var frameworkGroup = GetBestDependencyFrameworkGroupForCurrentSettings(foundPackage);


Suggested change

var frameworkGroup = GetBestDependencyFrameworkGroupForCurrentSettings(foundPackage);

// uninstall all non manually installed dependencies that are not a dependency of another installed package

var frameworkGroup = GetBestDependencyFrameworkGroupForCurrentSettings(foundPackage);

JoC0de · 2023-07-16T19:15:47Z

src/NuGetForUnity/Editor/NugetHelper.cs

+                        var actualData = PackagesConfigFile.Packages.Find(pkg => pkg.Id == package.Id);
+                        if (actualData != null)
+                        {
+                            package.IsManuallyInstalled = actualData.IsManuallyInstalled;
+                        }


Suggested change

var actualData = PackagesConfigFile.Packages.Find(pkg => pkg.Id == package.Id);

if (actualData != null)

{

package.IsManuallyInstalled = actualData.IsManuallyInstalled;

}

package.IsManuallyInstalled = PackagesConfigFile.Packages.FirstOrDefault(pkg => pkg.Id == package.Id)?.IsManuallyInstalled ?? false;

I more like this "one liners"

src/NuGetForUnity/Editor/PackagesConfigFile.cs

src/NuGetForUnity/Editor/NugetWindow.cs

igor84 · 2023-07-17T11:00:30Z

The unit test that is failing tests the following:

Installs bootstrap 3.3.7 which depends on jQuery [1.9.1, 4.0.0).
Checks that jquery 1.9.1 is installed.
Upgrades jquery to 3.1.1.
Reinstalls bootstrap 3.3.7 and checks if jquery 3.1.1 is still installed.

But with new functionality uninstalling bootstrap uninstalls the jquery since it is a transitive dependency and also reinstalls the old version again. The question is what do we want to do when updating packages.

I guess the most correct option would be to update all the transitive dependencies that no other package depends on but only to a newer version, so if some dependency was already manually updated to a new version there is no need to downgrade it.

Or maybe if you manually update some transitive dependency we should just mark it as manually installed from then on?

igor84 · 2023-07-17T11:03:22Z

One more thing we considered is that people with old packages.config files when they update to this new version of NugetForUnity all their packages will be marked as transitive dependencies. They can, of course, manually mark some as manually installed afterwards.

The only other option that came to our mind is to detect when none of the packages are marked as manually installed and then to mark them all. What do you think?

JoC0de · 2023-07-17T20:53:16Z

One more thing we considered is that people with old packages.config files when they update to this new version of NugetForUnity all their packages will be marked as transitive dependencies. They can, of course, manually mark some as manually installed afterwards.

The only other option that came to our mind is to detect when none of the packages are marked as manually installed and then to mark them all. What do you think?

I think best way would be to mark all packages as manually installed that are not a dependency of any other installed package. So only the root packages in the dependency tree are considered manually installed. This way we mark the minimum number of packages as manually installed to get a state that lead to the current amount of packages being installed.

JoC0de · 2023-07-17T21:01:39Z

The unit test that is failing tests the following:

Installs bootstrap 3.3.7 which depends on jQuery [1.9.1, 4.0.0).

Checks that jquery 1.9.1 is installed.

Upgrades jquery to 3.1.1.

Reinstalls bootstrap 3.3.7 and checks if jquery 3.1.1 is still installed.

But with new functionality uninstalling bootstrap uninstalls the jquery since it is a transitive dependency and also reinstalls the old version again. The question is what do we want to do when updating packages.

I guess the most correct option would be to update all the transitive dependencies that no other package depends on but only to a newer version, so if some dependency was already manually updated to a new version there is no need to downgrade it.

Or maybe if you manually update some transitive dependency we should just mark it as manually installed from then on?

Updating a package always ensures that the minimum version of all its dependencies are also installed so the only issue here is that the test expects jquery to not being uninstalled. I think uninstalling jquery is the scorest behavior, even if the package is updated manually, the user has the possibility to mark it as manually installed to prevent this. So in the UnitTest I would just mark jquery as manually installed so the test can still check the dependency installation.

popara96 · 2023-07-20T12:10:19Z

I decided to merge this because I think we agreed on all or almost all points, and all of the suggested fixes were implemented. In case you do have some additional comments, I can fix them later. This way it will be easier to work on other issues as most of them would make conflict with these changes.

popara96 added 2 commits July 14, 2023 17:57

Updated gitignore

efbd03b

Added support for better handling of transitive dependencies

7be37da

JoC0de requested changes Jul 16, 2023

View reviewed changes

popara96 added 2 commits July 18, 2023 14:19

Fixed PR comments and tests

a0b9ffd

Fixed refreshing installed packages when adding as explicit

c2c4b00

popara96 merged commit 6a2fa48 into master Jul 20, 2023

popara96 deleted the smarter-dependencies branch July 20, 2023 12:08

JoC0de linked an issue Jul 24, 2023 that may be closed by this pull request

Unclear outcome for Unity's embedded packages. #388

Closed

This was referenced Jul 24, 2023

Unclear outcome for Unity's embedded packages. #388

Closed

[Feature] Give user better feedback when user try to install a library already installed in the engine #403

Closed

JoC0de mentioned this pull request Oct 6, 2023

Support PackageReference system, e.g. .csproj. #88

Open

JoC0de mentioned this pull request Oct 22, 2023

Allow uninstalling of dependencies #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for better handling of transitive dependencies #539

Added support for better handling of transitive dependencies #539

popara96 commented Jul 14, 2023

JoC0de left a comment

JoC0de Jul 16, 2023

JoC0de Jul 16, 2023

igor84 Jul 17, 2023

JoC0de Jul 17, 2023

JoC0de Jul 16, 2023

JoC0de Jul 16, 2023

JoC0de Jul 16, 2023

igor84 commented Jul 17, 2023

igor84 commented Jul 17, 2023

JoC0de commented Jul 17, 2023

JoC0de commented Jul 17, 2023

popara96 commented Jul 20, 2023

	var frameworkGroup = GetBestDependencyFrameworkGroupForCurrentSettings(foundPackage);
	// uninstall all non manually installed dependencies that are not a dependency of another installed package
	var frameworkGroup = GetBestDependencyFrameworkGroupForCurrentSettings(foundPackage);

Added support for better handling of transitive dependencies #539

Added support for better handling of transitive dependencies #539

Conversation

popara96 commented Jul 14, 2023

JoC0de left a comment

Choose a reason for hiding this comment

JoC0de Jul 16, 2023

Choose a reason for hiding this comment

JoC0de Jul 16, 2023

Choose a reason for hiding this comment

igor84 Jul 17, 2023

Choose a reason for hiding this comment

JoC0de Jul 17, 2023

Choose a reason for hiding this comment

JoC0de Jul 16, 2023

Choose a reason for hiding this comment

JoC0de Jul 16, 2023

Choose a reason for hiding this comment

JoC0de Jul 16, 2023

Choose a reason for hiding this comment

igor84 commented Jul 17, 2023

igor84 commented Jul 17, 2023

JoC0de commented Jul 17, 2023

JoC0de commented Jul 17, 2023

popara96 commented Jul 20, 2023