| Message ID | 20200922210101.4081073-5-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
On Tue, Sep 22, 2020 at 05:00:27PM -0400, John Snow wrote: > All of the QAPI include statements are changed to be package-aware, as > explicit relative imports. > > A quirk of Python packages is that the name of the package exists only > *outside* of the package. This means that to a module inside of the qapi > folder, there is inherently no such thing as the "qapi" package. The > reason these imports work is because the "qapi" package exists in the > context of the caller -- the execution shim, where sys.path includes a > directory that has a 'qapi' folder in it. > > When we write "from qapi import sibling", we are NOT referencing the folder > 'qapi', but rather "any package named qapi in sys.path". If you should > so happen to have a 'qapi' package in your path, it will use *that* > package. > > When we write "from .sibling import foo", we always reference explicitly > our sibling module; guaranteeing consistency in *where* we are importing > these modules from. > > This can be useful when working with virtual environments and packages > in development mode. In development mode, a package is installed as a > series of symlinks that forwards to your same source files. The problem > arises because code quality checkers will follow "import qapi.x" to the > "installed" version instead of the sibling file and -- even though they > are the same file -- they have different module paths, and this causes > cyclic import problems, false positive type mismatch errors, and more. > > It can also be useful when dealing with hierarchical packages, e.g. if > we allow qemu.core.qmp, qemu.qapi.parser, etc. > > Signed-off-by: John Snow <jsnow@redhat.com> Here's the answer to the question I sent on patch 03/38. :) Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> -- Eduardo
On Tue, Sep 22, 2020 at 05:00:27PM -0400, John Snow wrote: > All of the QAPI include statements are changed to be package-aware, as > explicit relative imports. > > A quirk of Python packages is that the name of the package exists only > *outside* of the package. This means that to a module inside of the qapi > folder, there is inherently no such thing as the "qapi" package. The > reason these imports work is because the "qapi" package exists in the > context of the caller -- the execution shim, where sys.path includes a > directory that has a 'qapi' folder in it. > > When we write "from qapi import sibling", we are NOT referencing the folder > 'qapi', but rather "any package named qapi in sys.path". If you should > so happen to have a 'qapi' package in your path, it will use *that* > package. > > When we write "from .sibling import foo", we always reference explicitly > our sibling module; guaranteeing consistency in *where* we are importing > these modules from. > > This can be useful when working with virtual environments and packages > in development mode. In development mode, a package is installed as a > series of symlinks that forwards to your same source files. The problem > arises because code quality checkers will follow "import qapi.x" to the > "installed" version instead of the sibling file and -- even though they > are the same file -- they have different module paths, and this causes > cyclic import problems, false positive type mismatch errors, and more. > > It can also be useful when dealing with hierarchical packages, e.g. if > we allow qemu.core.qmp, qemu.qapi.parser, etc. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/commands.py | 4 ++-- > scripts/qapi/doc.py | 2 +- > scripts/qapi/events.py | 8 ++++---- > scripts/qapi/expr.py | 4 ++-- > scripts/qapi/gen.py | 4 ++-- > scripts/qapi/introspect.py | 8 ++++---- > scripts/qapi/main.py | 16 ++++++++-------- > scripts/qapi/parser.py | 4 ++-- > scripts/qapi/schema.py | 8 ++++---- > scripts/qapi/types.py | 6 +++--- > scripts/qapi/visit.py | 6 +++--- > 11 files changed, 35 insertions(+), 35 deletions(-) > Relative imports are a source of heated debates, but when properly used in a self contained module like here, they are very posititive IMO. Reviewed-by: Cleber Rosa <crosa@redhat.com>
On 9/23/20 9:18 AM, Cleber Rosa wrote: > Relative imports are a source of heated debates, but when properly > used in a self contained module like here, they are very posititive > IMO. Still? I know they were loathed pre-3.5, but in my subjective experience they behave the nicest overall in the modern python dialect. What are the downsides? --js
On Wed, Sep 23, 2020 at 01:12:09PM -0400, John Snow wrote: > On 9/23/20 9:18 AM, Cleber Rosa wrote: > > Relative imports are a source of heated debates, but when properly > > used in a self contained module like here, they are very posititive > > IMO. > > Still? I know they were loathed pre-3.5, but in my subjective experience > they behave the nicest overall in the modern python dialect. > > What are the downsides? > > --js I'll just invite Beraldo to the discussion and let the fun begin :). - Cleber.
On Thu, Sep 24, 2020 at 03:25:50PM -0400, Cleber Rosa wrote: > On Wed, Sep 23, 2020 at 01:12:09PM -0400, John Snow wrote: > > On 9/23/20 9:18 AM, Cleber Rosa wrote: > > > Relative imports are a source of heated debates, but when properly > > > used in a self contained module like here, they are very posititive > > > IMO. > > > > Still? I know they were loathed pre-3.5, but in my subjective experience > > they behave the nicest overall in the modern python dialect. > > > > What are the downsides? > > > > --js > > I'll just invite Beraldo to the discussion and let the fun begin :). Nice try, Cleber! ;) Well, relative imports are supported by Guido, so I'm not here to say different. There are some use-cases. I'm not fully aware of the qapi context and big picture here, but I guess that depends on how you would like to use your package/scripts. Some may say that one "downside" is that relative imports are not as readable as absolute ones. But reading the 04/38 PATH description by jsnow, yes, looks like using relative imports is one valid option here. I prefer to use my scripts as packages inside venvs, and I use to have a setup.py, with absolute imports whenever possible, and when in development mode, make use of `python3 setup.py develop` which will create the "links" for me. -- Beraldo
On Thu, Sep 24, 2020 at 07:17:47PM -0300, Beraldo Leal wrote: > On Thu, Sep 24, 2020 at 03:25:50PM -0400, Cleber Rosa wrote: > > On Wed, Sep 23, 2020 at 01:12:09PM -0400, John Snow wrote: > > > On 9/23/20 9:18 AM, Cleber Rosa wrote: > > > > Relative imports are a source of heated debates, but when properly > > > > used in a self contained module like here, they are very posititive > > > > IMO. > > > > > > Still? I know they were loathed pre-3.5, but in my subjective experience > > > they behave the nicest overall in the modern python dialect. > > > > > > What are the downsides? > > > > > > --js > > > > I'll just invite Beraldo to the discussion and let the fun begin :). > > Nice try, Cleber! ;) > C'mon... I was hoping for nothing less than an emacs .vs. vi kind of discussion. > Well, relative imports are supported by Guido, so I'm not here to say > different. There are some use-cases. > > I'm not fully aware of the qapi context and big picture here, but I > guess that depends on how you would like to use your package/scripts. > > Some may say that one "downside" is that relative imports are not as > readable as absolute ones. But reading the 04/38 PATH description by > jsnow, yes, looks like using relative imports is one valid option here. > > I prefer to use my scripts as packages inside venvs, and I use to have a > setup.py, with absolute imports whenever possible, and when in > development mode, make use of `python3 setup.py develop` which will > create the "links" for me. > Now seriously, these are good point, thanks. John, I invited Beraldo to give his take on the subject here because he started this issue on Avocado land: https://github.com/avocado-framework/avocado/issues/3525 So far on Avocado we've kept the relative imports on most places. We do have some occurences of "triple upper level" imports that don't look very nice IMO though: https://github.com/avocado-framework/avocado/blob/master/avocado/utils/software_manager/backends/rpm.py#L5 > -- > Beraldo > Anyway, I think we're all in agreement with the approach taken here. - Cleber.
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 3cf9e1110b..ce5926146a 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -13,8 +13,8 @@ See the COPYING file in the top-level directory. """ -from qapi.common import * -from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext +from .common import * +from .gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext def gen_command_decl(name, arg_type, boxed, ret_type): diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py index 92f584edcf..cbf7076ed9 100644 --- a/scripts/qapi/doc.py +++ b/scripts/qapi/doc.py @@ -5,7 +5,7 @@ """This script produces the documentation of a qapi schema in texinfo format""" import re -from qapi.gen import QAPIGenDoc, QAPISchemaVisitor +from .gen import QAPIGenDoc, QAPISchemaVisitor MSG_FMT = """ diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index b544af5a1c..0467272438 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -12,10 +12,10 @@ See the COPYING file in the top-level directory. """ -from qapi.common import * -from qapi.gen import QAPISchemaModularCVisitor, ifcontext -from qapi.schema import QAPISchemaEnumMember -from qapi.types import gen_enum, gen_enum_lookup +from .common import * +from .gen import QAPISchemaModularCVisitor, ifcontext +from .schema import QAPISchemaEnumMember +from .types import gen_enum, gen_enum_lookup def build_event_send_proto(name, arg_type, boxed): diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 2942520399..03b31ecfc1 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -16,8 +16,8 @@ import re from collections import OrderedDict -from qapi.common import c_name -from qapi.error import QAPISemError +from .common import c_name +from .error import QAPISemError # Names must be letters, numbers, -, and _. They must start with letter, diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index bf5552a4e7..8df19a0df0 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -17,8 +17,8 @@ import re from contextlib import contextmanager -from qapi.common import * -from qapi.schema import QAPISchemaVisitor +from .common import * +from .schema import QAPISchemaVisitor class QAPIGen: diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 23652be810..2a34cd1e8e 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -10,10 +10,10 @@ See the COPYING file in the top-level directory. """ -from qapi.common import * -from qapi.gen import QAPISchemaMonolithicCVisitor -from qapi.schema import (QAPISchemaArrayType, QAPISchemaBuiltinType, - QAPISchemaType) +from .common import * +from .gen import QAPISchemaMonolithicCVisitor +from .schema import (QAPISchemaArrayType, QAPISchemaBuiltinType, + QAPISchemaType) def _make_tree(obj, ifcond, features, extra=None): diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py index 18c246bbb4..3f8338ade8 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -11,14 +11,14 @@ import re import sys -from qapi.commands import gen_commands -from qapi.doc import gen_doc -from qapi.error import QAPIError -from qapi.events import gen_events -from qapi.introspect import gen_introspect -from qapi.schema import QAPISchema -from qapi.types import gen_types -from qapi.visit import gen_visit +from .commands import gen_commands +from .doc import gen_doc +from .error import QAPIError +from .events import gen_events +from .introspect import gen_introspect +from .schema import QAPISchema +from .types import gen_types +from .visit import gen_visit DEFAULT_OUTPUT_DIR = '' diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 165925ca72..327cf05736 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -18,8 +18,8 @@ import re from collections import OrderedDict -from qapi.error import QAPIParseError, QAPISemError -from qapi.source import QAPISourceInfo +from .error import QAPIParseError, QAPISemError +from .source import QAPISourceInfo class QAPISchemaParser: diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 78309a00f0..a835ee6fde 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -18,10 +18,10 @@ import re from collections import OrderedDict -from qapi.common import c_name, pointer_suffix -from qapi.error import QAPIError, QAPISemError -from qapi.expr import check_exprs -from qapi.parser import QAPISchemaParser +from .common import c_name, pointer_suffix +from .error import QAPIError, QAPISemError +from .expr import check_exprs +from .parser import QAPISchemaParser class QAPISchemaEntity: diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 3640f17cd6..ca9a5aacb3 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -13,9 +13,9 @@ # See the COPYING file in the top-level directory. """ -from qapi.common import * -from qapi.gen import QAPISchemaModularCVisitor, ifcontext -from qapi.schema import QAPISchemaEnumMember, QAPISchemaObjectType +from .common import * +from .gen import QAPISchemaModularCVisitor, ifcontext +from .schema import QAPISchemaEnumMember, QAPISchemaObjectType # variants must be emitted before their container; track what has already diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index cdabc5fa28..7850f6e848 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -13,9 +13,9 @@ See the COPYING file in the top-level directory. """ -from qapi.common import * -from qapi.gen import QAPISchemaModularCVisitor, ifcontext -from qapi.schema import QAPISchemaObjectType +from .common import * +from .gen import QAPISchemaModularCVisitor, ifcontext +from .schema import QAPISchemaObjectType def gen_visit_decl(name, scalar=False):
All of the QAPI include statements are changed to be package-aware, as explicit relative imports. A quirk of Python packages is that the name of the package exists only *outside* of the package. This means that to a module inside of the qapi folder, there is inherently no such thing as the "qapi" package. The reason these imports work is because the "qapi" package exists in the context of the caller -- the execution shim, where sys.path includes a directory that has a 'qapi' folder in it. When we write "from qapi import sibling", we are NOT referencing the folder 'qapi', but rather "any package named qapi in sys.path". If you should so happen to have a 'qapi' package in your path, it will use *that* package. When we write "from .sibling import foo", we always reference explicitly our sibling module; guaranteeing consistency in *where* we are importing these modules from. This can be useful when working with virtual environments and packages in development mode. In development mode, a package is installed as a series of symlinks that forwards to your same source files. The problem arises because code quality checkers will follow "import qapi.x" to the "installed" version instead of the sibling file and -- even though they are the same file -- they have different module paths, and this causes cyclic import problems, false positive type mismatch errors, and more. It can also be useful when dealing with hierarchical packages, e.g. if we allow qemu.core.qmp, qemu.qapi.parser, etc. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/commands.py | 4 ++-- scripts/qapi/doc.py | 2 +- scripts/qapi/events.py | 8 ++++---- scripts/qapi/expr.py | 4 ++-- scripts/qapi/gen.py | 4 ++-- scripts/qapi/introspect.py | 8 ++++---- scripts/qapi/main.py | 16 ++++++++-------- scripts/qapi/parser.py | 4 ++-- scripts/qapi/schema.py | 8 ++++---- scripts/qapi/types.py | 6 +++--- scripts/qapi/visit.py | 6 +++--- 11 files changed, 35 insertions(+), 35 deletions(-)