[LNT][NFC] Fix global import in function

[thopre]

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.

[thopre]

Ping?

[thopre]

Ping?

[cmatthews]

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

[thopre]

In D68779#1741055, @cmatthews wrote:

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]

In D68779#1741077, @thopre wrote:

In D68779#1741055, @cmatthews wrote:

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?

[thopre]

In D68779#1741898, @thopre wrote:

In D68779#1741077, @thopre wrote:

In D68779#1741055, @cmatthews wrote:

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.

[thopre]

In D68779#1743986, @thopre wrote:

In D68779#1741898, @thopre wrote:

In D68779#1741077, @thopre wrote:

In D68779#1741055, @cmatthews wrote:

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?

[thopre]

! 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?

[PrzemekWirkus]

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

[PrzemekWirkus]

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]

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]

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]

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]

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]

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

[cmatthews]

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]

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

[thopre]

In D68779#1771332, @cmatthews wrote:

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]

@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]

Rebase later in patch series

[thopre]

Ping @cmatthews ?

[thopre]

In D68779#1811800, @thopre wrote:

Ping @cmatthews ?

Ping?

[PrzemekWirkus]

For Python 2 only support in LNT this patch LGTM.

[thopre]

Committed as 10f1592ecb34bdbf2d7c62736aaf987df2fdd511

lnt/lnttool/admin.py

 #!/usr/bin/env python
 from future import standard_library
 standard_library.install_aliases()
 import click
+from importlib import import_module
 from .common import submit_options
 
 
 def _load_dependencies():
     global yaml, sys, requests, json, os, http
-    import yaml
-    import sys
-    import requests
-    import json
-    import os
-    import http.client
+    yaml = import_module('yaml')
+    sys = import_module('sys')
+    requests = import_module('requests')
+    json = import_module('json')
+    os = import_module('os')
+    http = import_module('http.client')
 
 
 def _error(msg):
     sys.stderr.write('%s\n' % msg)
 
 
 def _fatal(msg):
     _error(msg)
     sys.exit(1)