Re: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies

["Bret Barkelew" <bret.barkelew@microsoft.com>]

[-- Attachment #1: Type: text/plain, Size: 14480 bytes --]

Spiffy. Agreed; thanks for the background.

My only other concern would be the function name and if some future developer thought that was permission to add other modules to it.
Then again, maybe I’m just bikeshedding. Feel free to ignore.

- Bret

From: Desimone, Ashley E<mailto:ashley.e.desimone@intel.com>
Sent: Friday, March 20, 2020 1:21 PM
To: Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Pandya, Puja<mailto:puja.pandya@intel.com>; Bjorge, Erik C<mailto:erik.c.bjorge@intel.com>
Subject: [EXTERNAL] RE: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies

Thanks for the explanation.

Reviewed-by: Ashley DeSimone <ashley.e.deismone@intel.com>

From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: Thursday, March 19, 2020 2:01 PM
To: Bret Barkelew <Bret.Barkelew@microsoft.com>; Desimone, Ashley E <ashley.e.desimone@intel.com>; devel@edk2.groups.io
Cc: Pandya, Puja <puja.pandya@intel.com>; Bjorge, Erik C <erik.c.bjorge@intel.com>
Subject: Re: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies

Hi Bret,

I alluded to this a little bit in the cover letter, completely agreed a more detailed explanation is needed.

There is an odd situation with the way GitPython’s dependencies are evolved over time. Initially, they had the following dependency graph:


  *   GitPython
     *   gitdb
        *   smmap

At some point in the past, they decided to make backwards incompatible API changes to gitdb and smmap, so the dependencies where updated as follows:


  *   GitPython
     *   gitdb2
        *   smmap2

However, now that Python 2.x is deprecated, any code that was using the old API will need updates anyway. So, the GitPython maintainers decided to go back to the original dependency tree. The issue is, gitdb2 and smmap2 wheels create packages in site-packages with the names gitdb and smmap, so installing gitdb2 and gitdb at the same time results in the gitdb folder in site-packages being overwritten. End result pretty crappy, whichever wheel was most recently installed is the one that will prevail in site-packages.

The way the GitPython maintainers addressed this problem is by converting gitdb2 and smmap2 into wheels with no content other than a dependency on gitdb and smmap. So when one does a pip upgrade those packages remain and you get a dependency graph that looks like this:


  *   GitPython
     *   gitdb2
        *   smmap2
  *   gitdb2
     *   gitdb
        *   smmap
  *   smmap2
     *   smmap


So, the net result for a regular pip upgrade leaves a fair amount of detritus sitting on the system which would not be present for a new user doing a clean install from pip. Obviously, we can do better than that which is why I’ve automated the uninstallation of smmap2 and gitdb2 before proceeding to upgrade GitPython. This also guarantees that the duplicate folder in site-packages situation does not happen at the same time.

The fact that the GitPython maintainers created this situation in the first place is rather annoying.

Thanks,
Nate

From: Bret Barkelew <Bret.Barkelew@microsoft.com<mailto:Bret.Barkelew@microsoft.com>>
Date: Thursday, March 19, 2020 at 12:44 PM
To: Ashley E Desimone <ashley.e.desimone@intel.com<mailto:ashley.e.desimone@intel.com>>, "Desimone, Nathaniel L" <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>>, "devel@edk2.groups.io<mailto:devel@edk2.groups.io>" <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: "Pandya, Puja" <puja.pandya@intel.com<mailto:puja.pandya@intel.com>>, "Bjorge, Erik C" <erik.c.bjorge@intel.com<mailto:erik.c.bjorge@intel.com>>
Subject: RE: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies

Naïve question: why isn’t this upgrade handled when pip installs the new wheel? Are the dependencies not matched up correctly? I would think that pip would uninstall the old version on its own.

- Bret

From: Desimone, Ashley E<mailto:ashley.e.desimone@intel.com>
Sent: Thursday, March 19, 2020 12:32 PM
To: Desimone, Nathaniel L<mailto:nathaniel.l.desimone@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Pandya, Puja<mailto:puja.pandya@intel.com>; Bjorge, Erik C<mailto:erik.c.bjorge@intel.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Subject: [EXTERNAL] RE: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies

For the following section why are we singling out gitdb2 and smmap2 when deleting obsolete dependencies? Is there an alternative to hardcoding these values?

+                if (DeleteObsoletePackages)
+                {
+                    //
+                    // Delete obsolete dependencies
+                    //
+                    foreach (string PackageName in new string[] { "smmap2", "gitdb2" })
+                    {
+                        if (InstalledPackages.Where(p => p.Name == PackageName).FirstOrDefault() != null)
+                        {
+                            InstallLogger.Log(string.Format("Uninstalling {0}", PackageName));
+                            PythonOperations.UninstallPythonPackage(PythonPath, PackageName);
+                        }
+                    }
+                }

Thanks,
Ashley

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>>
Sent: Wednesday, March 18, 2020 8:17 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Desimone, Ashley E <ashley.e.desimone@intel.com<mailto:ashley.e.desimone@intel.com>>; Pandya, Puja <puja.pandya@intel.com<mailto:puja.pandya@intel.com>>; Bjorge, Erik C <erik.c.bjorge@intel.com<mailto:erik.c.bjorge@intel.com>>; Bret Barkelew <Bret.Barkelew@microsoft.com<mailto:Bret.Barkelew@microsoft.com>>
Subject: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies

Cc: Ashley DeSimone <ashley.e.desimone@intel.com<mailto:ashley.e.desimone@intel.com>>
Cc: Puja Pandya <puja.pandya@intel.com<mailto:puja.pandya@intel.com>>
Cc: Erik Bjorge <erik.c.bjorge@intel.com<mailto:erik.c.bjorge@intel.com>>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com<mailto:Bret.Barkelew@microsoft.com>>
Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>>
---
 .../EdkRepoInstaller/InstallWorker.cs         | 30 +++++++++-
 edkrepo_installer/linux-scripts/install.py    | 58 ++++++++++++-------
 2 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/edkrepo_installer/EdkRepoInstaller/InstallWorker.cs b/edkrepo_installer/EdkRepoInstaller/InstallWorker.cs
index 8d824f2..b44e8fa 100644
--- a/edkrepo_installer/EdkRepoInstaller/InstallWorker.cs
+++ b/edkrepo_installer/EdkRepoInstaller/InstallWorker.cs
@@ -710,6 +710,7 @@ namespace TianoCore.EdkRepoInstaller
                 string PythonPath;
                 bool Has32Bit;
                 bool Has64Bit;
+                bool DeleteObsoletePackages = false;
                 PythonOperations.GetPythonBitness(PyInstance.Version.Major, PyInstance.Version.Minor, out Has32Bit, out Has64Bit);
                 if (Has32Bit && Has64Bit && PyInstance.Architecture == CpuArchitecture.IA32)
                 {
@@ -720,14 +721,39 @@ namespace TianoCore.EdkRepoInstaller
                     PythonPath = PythonOperations.FindPython(PyInstance.Version.Major, PyInstance.Version.Minor, false);
                 }
                 List<PythonPackage> InstalledPackages = PythonOperations.GetInstalledPythonPackages(PythonPath);
-                foreach(PythonWheel Wheel in PyInstance.Wheels)
+                foreach (PythonWheel Wheel in PyInstance.Wheels)
+                {
+                    //If a package is already installed, check if we have a newer version bundled in the installer
+                    //If yes, the package will be upgraded, make sure obsolete packages are uninstalled first
+                    PythonPackage InstalledPackage = InstalledPackages.Where(p => p.Name == Wheel.Package.Name.Replace('_', '-')).FirstOrDefault();
+                    if (InstalledPackage != null && InstalledPackage.Version < Wheel.Package.Version)
+                    {
+                        DeleteObsoletePackages = true;
+                        break;
+                    }
+                }
+                if (DeleteObsoletePackages)
+                {
+                    //
+                    // Delete obsolete dependencies
+                    //
+                    foreach (string PackageName in new string[] { "smmap2", "gitdb2" })
+                    {
+                        if (InstalledPackages.Where(p => p.Name == PackageName).FirstOrDefault() != null)
+                        {
+                            InstallLogger.Log(string.Format("Uninstalling {0}", PackageName));
+                            PythonOperations.UninstallPythonPackage(PythonPath, PackageName);
+                        }
+                    }
+                }
+                foreach (PythonWheel Wheel in PyInstance.Wheels)
                 {
                     //PythonPackage InstalledPackage = (from package in InstalledPackages
                     //                                  where package.Name == Wheel.Package.Name
                     //                                  select package).FirstOrDefault();
                     //If the package is already installed, check if we have a newer version bundled in the installer
                     //If so, upgrade the package
-                    PythonPackage InstalledPackage = InstalledPackages.Where(p => p.Name == Wheel.Package.Name).FirstOrDefault();
+                    PythonPackage InstalledPackage =
+ InstalledPackages.Where(p => p.Name == Wheel.Package.Name.Replace('_',
+ '-')).FirstOrDefault();
                     if (InstalledPackage != null)
                     {
                         if (InstalledPackage.Version < Wheel.Package.Version) diff --git a/edkrepo_installer/linux-scripts/install.py b/edkrepo_installer/linux-scripts/install.py
index 0ea486d..b2cdfed 100755
--- a/edkrepo_installer/linux-scripts/install.py
+++ b/edkrepo_installer/linux-scripts/install.py
@@ -3,7 +3,7 @@
 ## @file
 # install.py
 #
-# Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2018 - 2020, Intel Corporation. All rights
+reserved.<BR>
 # SPDX-License-Identifier: BSD-2-Clause-Patent  #

@@ -53,6 +53,26 @@ def get_args():
     parser.add_argument('-v', '--verbose', action='store_true', default=False, help='Enables verbose output')
     return parser.parse_args()

+def get_installed_packages(python_command):
+    pip_cmd = [def_python, '-m', 'pip', 'list', '--legacy']
+    try:
+        res = default_run(pip_cmd)
+    except:
+        pip_cmd.pop(-1)
+        try:
+            res = default_run(pip_cmd)
+        except:
+            return ret_val
+    installed_packages = {}
+    for pip_mod in res.stdout.split('\n'):
+        try:
+            name, version = pip_mod.split()
+            version = version.strip().strip('()')
+        except:
+            continue
+        installed_packages[name] = version
+    return installed_packages
+
 def get_required_wheels():
     ret_val = collections.OrderedDict()
     if platform.machine() == 'x86_64':
@@ -79,27 +99,11 @@ def get_required_wheels():
                                                  'version':wheel.attrib['Version'],
                                                  'install':True}
             break
-    pip_cmd = [def_python, '-m', 'pip', 'list', '--legacy']
-    try:
-        res = default_run(pip_cmd)
-    except:
-        pip_cmd.pop(-1)
-        try:
-            res = default_run(pip_cmd)
-        except:
-            return ret_val
-    installed_modules = {}
-    for pip_mod in res.stdout.split('\n'):
-        try:
-            name, version = pip_mod.split()
-            version = version.strip().strip('()')
-        except:
-            continue
-        installed_modules[name] = version
+    installed_packages = get_installed_packages(def_python)
     for name in ret_val:
         #pip doesn't understand the difference between '_' and '-'
-        if name.replace('_','-') in installed_modules:
-            version = installed_modules[name.replace('_','-')]
+        if name.replace('_','-') in installed_packages:
+            version = installed_packages[name.replace('_','-')]
             if _check_version(version, ret_val[name]['version']) >= 0 and not ret_val[name]['uninstall']:
                 ret_val[name]['install'] = False
             else:
@@ -328,16 +332,30 @@ def do_install():
         if not os.path.isdir(whl_src_dir):
             log.info('- Missing wheel file directory')
             return 1
+        updating_edkrepo = False
         for whl_name in wheels_to_install:
             uninstall_whl = wheels_to_install[whl_name]['uninstall']
             whl_name = whl_name.replace('_','-')  #pip doesn't understand the difference between '_' and '-'
             if uninstall_whl:
+                updating_edkrepo = True
                 try:
                     res = default_run([def_python, '-m', 'pip', 'uninstall', '--yes', whl_name])
                 except:
                     log.info('- Failed to uninstall {}'.format(whl_name))
                     return 1
                 log.info('+ Uninstalled {}'.format(whl_name))
+
+        #Delete obsolete dependencies
+        if updating_edkrepo:
+            installed_packages = get_installed_packages(def_python)
+            for whl_name in ['smmap2', 'gitdb2']:
+                if whl_name in installed_packages:
+                    try:
+                        res = default_run([def_python, '-m', 'pip', 'uninstall', '--yes', whl_name])
+                    except:
+                        log.info('- Failed to uninstall {}'.format(whl_name))
+                        return 1
+                    log.info('+ Uninstalled {}'.format(whl_name))
         for whl_name in wheels_to_install:
             whl = wheels_to_install[whl_name]['wheel']
             install_whl = wheels_to_install[whl_name]['install']
--
2.24.0.windows.2



[-- Attachment #2: Type: text/html, Size: 35244 bytes --]