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 Date: Thursday, March 19, 2020 at 12:44 PM To: Ashley E Desimone , "Desimone, Nathaniel L" , "devel@edk2.groups.io" Cc: "Pandya, Puja" , "Bjorge, Erik C" 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 Sent: Thursday, March 19, 2020 12:32 PM To: Desimone, Nathaniel L; devel@edk2.groups.io Cc: Pandya, Puja; Bjorge, Erik C; Bret Barkelew 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 Sent: Wednesday, March 18, 2020 8:17 PM To: devel@edk2.groups.io Cc: Desimone, Ashley E ; Pandya, Puja ; Bjorge, Erik C ; Bret Barkelew Subject: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies Cc: Ashley DeSimone Cc: Puja Pandya Cc: Erik Bjorge Cc: Bret Barkelew Signed-off-by: Nate DeSimone --- .../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 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.
+# Copyright (c) 2018 - 2020, Intel Corporation. All rights +reserved.
# 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