Page MenuHomePhabricator

[LNT][NFC] Fix global import in function
ClosedPublic

Authored by thopre on Oct 10 2019, 4:05 AM.

Details

Summary

The _load_dependencies() function in lnt.lnttool.admin import some
modules in the global namespace by making the module name global and
then doing imports. However the language reference for global statements
forbids this:

"Names listed in a global statement must not be defined as formal
parameters or in a for loop control target, class definition, function
definition, or import statement."

This commit makes use of importlib.import_module wrapper instead of the
import statement to respect this restriction.

Event Timeline

thopre created this revision.Oct 10 2019, 4:05 AM
thopre retitled this revision from [LNT] Fix global import in function to [LNT][NFC] Fix global import in function.Oct 12 2019, 4:02 PM

I don't understand why we are not using normal import at the top level here?

I don't understand why we are not using normal import at the top level here?

According to a comment in the file it's done this way to load dependencies lazily. It does not say why it is desirable to do so though. Since the same dependencies are loaded for all admin subcommand I'd say it's probably not necessary but I'd prefer to fix that in a separate patch once all Python 3 support patches are in to keep this one a NFC.

thopre added a subscriber: MatzeB.Nov 12 2019, 2:15 AM

I don't understand why we are not using normal import at the top level here?

According to a comment in the file it's done this way to load dependencies lazily. It does not say why it is desirable to do so though. Since the same dependencies are loaded for all admin subcommand I'd say it's probably not necessary but I'd prefer to fix that in a separate patch once all Python 3 support patches are in to keep this one a NFC.

@MatzeB Can you confirm what is the purpose of loading lnt admin dependencies lazily?

I don't understand why we are not using normal import at the top level here?

According to a comment in the file it's done this way to load dependencies lazily. It does not say why it is desirable to do so though. Since the same dependencies are loaded for all admin subcommand I'd say it's probably not necessary but I'd prefer to fix that in a separate patch once all Python 3 support patches are in to keep this one a NFC.

@MatzeB Can you confirm what is the purpose of loading lnt admin dependencies lazily?

Matthias replied saying the point was to avoid adding some extra package requirement for the client side of LNT. So I think the patch stands as it is.

I don't understand why we are not using normal import at the top level here?

According to a comment in the file it's done this way to load dependencies lazily. It does not say why it is desirable to do so though. Since the same dependencies are loaded for all admin subcommand I'd say it's probably not necessary but I'd prefer to fix that in a separate patch once all Python 3 support patches are in to keep this one a NFC.

@MatzeB Can you confirm what is the purpose of loading lnt admin dependencies lazily?

Matthias replied saying the point was to avoid adding some extra package requirement for the client side of LNT. So I think the patch stands as it is.

@hubert.reinterpretcast @cmatthews Ping?

! In D68779#1743986, @thopre wrote:

! In D68779#1741898, @thopre wrote:

@MatzeB Can you confirm what is the purpose of loading lnt admin dependencies lazily?

Matthias replied saying the point was to avoid adding some extra package requirement for the client side of LNT. So I think the patch stands as it is.

@hubert.reinterpretcast @cmatthews Ping?

@thopre Thomas, please add me and @leandron as reviewers to this series of patches. We can have a look.

PrzemekWirkus marked an inline comment as done.Dec 4 2019, 6:43 AM
PrzemekWirkus added inline comments.
lnt/lnttool/admin.py
11–17

Thomas,
why are you not using importlib.import_module instead of import ?
Any other reason except for the Python 2 backward compatibility ?

11–17

s/import/__import__/

thopre marked an inline comment as done.Dec 4 2019, 6:48 AM
thopre added inline comments.
lnt/lnttool/admin.py
11–17

The version available in Python 2.7 does not seem to return a value, how to bind the loaded modules to the global variables then?

PrzemekWirkus added inline comments.Dec 5 2019, 4:26 AM
lnt/lnttool/admin.py
11–17

Python 2.7.15+ (default, Oct 7 2019, 17:39:04)
[GCC 7.4.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.

import importlib
x = importlib.import_module('json')
print x

<module 'json' from '/usr/lib/python2.7/json/__init__.pyc'>

Silly global assign:

global json
json = x
dir(json)

['JSONDecoder', 'JSONEncoder', 'all', 'author', 'builtins', 'doc', 'file', 'name', 'package', 'path', 'version', '_default_decoder', '_default_encoder', 'decoder', 'dump', 'dumps', 'encoder', 'load', 'loads', 'scanner']

https://docs.python.org/2/library/importlib.html#importlib.import_module

"...
The specified module will be inserted into sys.modules and returned."

11–17

This is just nitpicking from my side! :)

thopre marked 5 inline comments as done.Dec 5 2019, 4:50 AM
thopre added inline comments.
lnt/lnttool/admin.py
11–17

Ah, I missed the "and returned". Thanks for pointing that out. It seems that importlib is only available in Python 3.1 onwards. Are you aware of any Linux distribution (past or present, e.g. RHELs) that ship with Python 3.0?

PrzemekWirkus added inline comments.Dec 5 2019, 7:03 AM
lnt/lnttool/admin.py
11–17

No one uses Python <3.4.3

bash
$ docker run --rm ubuntu:14.04 python3 --version
Python 3.4.3
bash
$ docker run --rm -it centos:6
$ yum update
$ yum install centos-release-scl
$ rpm -qa | grep python3
rh-python35-python-setuptools-18.0.1-2.el6.noarch
rh-python35-python-3.5.1-13.el6.x86_64
rh-python35-python-devel-3.5.1-13.el6.x86_64
rh-python35-2.0-2.el6.x86_64
rh-python35-runtime-2.0-2.el6.x86_64
rh-python35-python-pip-7.1.0-2.el6.noarch
rh-python35-python-libs-3.5.1-13.el6.x86_64
rh-python35-python-virtualenv-13.1.2-2.el6.noarch
thopre updated this revision to Diff 232342.Dec 5 2019, 7:19 AM
thopre marked an inline comment as done.

Use importlib.import_module instead of import, bumping Python 3.x requirements

I think we need testing on all versions of python we support. I suggest one version on 3 that is recent only. We currently have no python3 users, so maybe just 3.6 and newer?

thopre updated this revision to Diff 232427.Dec 5 2019, 12:03 PM

Remove Python 3 requirement since LNT is not Python 3 compatible at this stage

thopre edited the summary of this revision. (Show Details)Dec 5 2019, 12:04 PM

I think we need testing on all versions of python we support. I suggest one version on 3 that is recent only. We currently have no python3 users, so maybe just 3.6 and newer?

Sounds good. We can lower if we feel necessary later on. Since the LNT codebase is not Python 3 compatible at this point, I've removed the Python 3 version check from this patch and moved it into a separate patch later in the patch series once Python 3 compatibility is achieved and testing with Python3 is enabled: https://reviews.llvm.org/D71085

thopre added a comment.Dec 9 2019, 6:03 AM

@cmatthews Are you happy with this patch as it is? Changes proposed will require Python 3.1 which is covered by D71085 to require Python 2.7 or 3.6+ once all Python 3 patches are in. As to the unusual import Matthias explain it is to avoid adding unnecessary python module requirements for users of the LNT client only.

thopre updated this revision to Diff 233046.Dec 10 2019, 4:16 AM

Rebase later in patch series

PrzemekWirkus accepted this revision.Jan 17 2020, 6:59 AM

For Python 2 only support in LNT this patch LGTM.

This revision is now accepted and ready to land.Jan 17 2020, 6:59 AM
thopre closed this revision.Jan 17 2020, 11:04 AM

Committed as 10f1592ecb34bdbf2d7c62736aaf987df2fdd511