<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="#954F72"> <div class="WordSection1"> Seems fair. Thanks for the head up. 😊 <o:p> </o:p> - Bret <o:p> </o:p> </div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Nate DeSimone via Groups.Io <nathaniel.l.desimone=intel.com@groups.io> Sent: Saturday, March 21, 2020 2:56:20 PM To: Bret Barkelew <Bret.Barkelew@microsoft.com>; Desimone, Ashley E <ashley.e.desimone@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io> Cc: Pandya, Puja <puja.pandya@intel.com>; Bjorge, Erik C <erik.c.bjorge@intel.com> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies <div> </div> </div> <style>  </style> <div lang="EN-US" link="blue" vlink="#954F72"> <div class="x_WordSection1"> I’m reading between the lines a little bit… but it seems like your response amounts to: Acked-by: Bret Barkelew <Bret.Barkelew@microsoft.com> Which I added to the commit: <a href="https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-staging%2Fcommit%2F79ba022e&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C7844aa42c0d54d01cec208d7cde2b278%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637204245854647261&sdata=yteA9kEe%2FhNAWjDNYorj9sviOfcinyr%2BkZGgYHGWKwM%3D&reserved=0" originalsrc="https://github.com/tianocore/edk2-staging/commit/79ba022e" shash="OiV/rsRBCUiZSg0O59EfzNb6Wvk8Q9fSLbgHXzfgEwuShXnTEhurJ4UNAwRM6KTprJvIZOnJ+dLp0InwLqNM39auUOD7B9PHG8BtxfCALzi6FIN5viCN7eV3m92DjDvqFzvdRTJ+2WzOzFSowqV9uS2qCpPF13WQGMUBj04drO0="> https://github.com/tianocore/edk2-staging/commit/79ba022e</a> Hope that’s cool 😊. <div> <div style="border:none; border-top:solid #E1E1E1 1.0pt; padding:3.0pt 0in 0in 0in"> <a name="x______replyseparator"></a>From: Bret Barkelew <Bret.Barkelew@microsoft.com> Sent: Friday, March 20, 2020 3:07 PM To: Desimone, Ashley E <ashley.e.desimone@intel.com>; Desimone, Nathaniel L <nathaniel.l.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 </div> </div> 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 <div style="border:none; border-top:solid #E1E1E1 1.0pt; padding:3.0pt 0in 0in 0in"> From: <a href="mailto:ashley.e.desimone@intel.com">Desimone, Ashley E</a> Sent: Friday, March 20, 2020 1:21 PM To: <a href="mailto:nathaniel.l.desimone@intel.com">Desimone, Nathaniel L</a>; <a href="mailto:Bret.Barkelew@microsoft.com">Bret Barkelew</a>; <a href="mailto:devel@edk2.groups.io"> devel@edk2.groups.io</a> Cc: <a href="mailto:puja.pandya@intel.com">Pandya, Puja</a>; <a href="mailto:erik.c.bjorge@intel.com"> Bjorge, Erik C</a> Subject: [EXTERNAL] RE: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies </div> Thanks for the explanation. Reviewed-by: Ashley DeSimone <<a href="mailto:ashley.e.deismone@intel.com">ashley.e.deismone@intel.com</a>> <div> <div style="border:none; border-top:solid #E1E1E1 1.0pt; padding:3.0pt 0in 0in 0in"> From: Desimone, Nathaniel L <<a href="mailto:nathaniel.l.desimone@intel.com">nathaniel.l.desimone@intel.com</a>> Sent: Thursday, March 19, 2020 2:01 PM To: Bret Barkelew <<a href="mailto:Bret.Barkelew@microsoft.com">Bret.Barkelew@microsoft.com</a>>; Desimone, Ashley E <<a href="mailto:ashley.e.desimone@intel.com">ashley.e.desimone@intel.com</a>>; <a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a> Cc: Pandya, Puja <<a href="mailto:puja.pandya@intel.com">puja.pandya@intel.com</a>>; Bjorge, Erik C <<a href="mailto:erik.c.bjorge@intel.com">erik.c.bjorge@intel.com</a>> Subject: Re: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies </div> </div> 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: <ul type="disc" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">GitPython <ul type="circle" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">gitdb <ul type="square" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">smmap</li></ul> </li></ul> </li></ul> 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: <ul type="disc" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">GitPython <ul type="circle" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">gitdb2 <ul type="square" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">smmap2</li></ul> </li></ul> </li></ul> 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: <ul type="disc" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">GitPython <ul type="circle" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">gitdb2 <ul type="square" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">smmap2</li></ul> </li></ul> </li><li class="x_MsoListParagraph" style="margin-left:0in">gitdb2 <ul type="circle" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">gitdb <ul type="square" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">smmap</li></ul> </li></ul> </li><li class="x_MsoListParagraph" style="margin-left:0in">smmap2 <ul type="circle" style="margin-top:0in"> <li class="x_MsoListParagraph" style="margin-left:0in">smmap</li></ul> </li></ul> 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 <div style="border:none; border-top:solid #B5C4DF 1.0pt; padding:3.0pt 0in 0in 0in"> From: Bret Barkelew <<a href="mailto:Bret.Barkelew@microsoft.com">Bret.Barkelew@microsoft.com</a>> Date: Thursday, March 19, 2020 at 12:44 PM To: Ashley E Desimone <<a href="mailto:ashley.e.desimone@intel.com">ashley.e.desimone@intel.com</a>>, "Desimone, Nathaniel L" <<a href="mailto:nathaniel.l.desimone@intel.com">nathaniel.l.desimone@intel.com</a>>, "<a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a>" <<a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a>> Cc: "Pandya, Puja" <<a href="mailto:puja.pandya@intel.com">puja.pandya@intel.com</a>>, "Bjorge, Erik C" <<a href="mailto:erik.c.bjorge@intel.com">erik.c.bjorge@intel.com</a>> Subject: RE: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies </div> <div> </div> 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 <div style="border:none; border-top:solid #E1E1E1 1.0pt; padding:3.0pt 0in 0in 0in"> From: <a href="mailto:ashley.e.desimone@intel.com">Desimone, Ashley E</a> Sent: Thursday, March 19, 2020 12:32 PM To: <a href="mailto:nathaniel.l.desimone@intel.com">Desimone, Nathaniel L</a>; <a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a> Cc: <a href="mailto:puja.pandya@intel.com">Pandya, Puja</a>; <a href="mailto:erik.c.bjorge@intel.com"> Bjorge, Erik C</a>; <a href="mailto:Bret.Barkelew@microsoft.com">Bret Barkelew</a> Subject: [EXTERNAL] RE: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies </div> 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 <<a href="mailto:nathaniel.l.desimone@intel.com">nathaniel.l.desimone@intel.com</a>> Sent: Wednesday, March 18, 2020 8:17 PM To: <a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a> Cc: Desimone, Ashley E <<a href="mailto:ashley.e.desimone@intel.com">ashley.e.desimone@intel.com</a>>; Pandya, Puja <<a href="mailto:puja.pandya@intel.com">puja.pandya@intel.com</a>>; Bjorge, Erik C <<a href="mailto:erik.c.bjorge@intel.com">erik.c.bjorge@intel.com</a>>; Bret Barkelew <<a href="mailto:Bret.Barkelew@microsoft.com">Bret.Barkelew@microsoft.com</a>> Subject: [PATCH 1/4] EdkRepo: Installer should remove obsolete dependencies Cc: Ashley DeSimone <<a href="mailto:ashley.e.desimone@intel.com">ashley.e.desimone@intel.com</a>> Cc: Puja Pandya <<a href="mailto:puja.pandya@intel.com">puja.pandya@intel.com</a>> Cc: Erik Bjorge <<a href="mailto:erik.c.bjorge@intel.com">erik.c.bjorge@intel.com</a>> Cc: Bret Barkelew <<a href="mailto:Bret.Barkelew@microsoft.com">Bret.Barkelew@microsoft.com</a>> Signed-off-by: Nate DeSimone <<a href="mailto:nathaniel.l.desimone@intel.com">nathaniel.l.desimone@intel.com</a>> --- .../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. +# 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 </div> </div> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links: You receive all messages sent to this group. <a target="_blank" href="https://edk2.groups.io/g/devel/message/56079">View/Reply Online (#56079)</a> | | <a target="_blank" href="https://groups.io/mt/72067009/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com] <div width="1" style="color:white;clear:both">_._,_._,_</div>