aurzenligl / prophy

prophy: fast serialization protocol
MIT License
13 stars 8 forks source link

fix python3 from ... import ... statement #25

Closed jqb1 closed 4 years ago

jqb1 commented 5 years ago

Generated import statements are not valid in python 3

kamichal commented 5 years ago

from .module import symbol can also be invalid if given directory doesn't contain __init__.py. It's considerable if prophyc is supposed to generate empty __init__.py file in options.python_out directory path. It would require to add post-processing hook in PythonGenerator that appends empty string to optionally existing <options.python_out>/__init__.py file.

aurzenligl commented 5 years ago

This assumes generated file would always be used as a part of package, otherwise from .(...) import would be meaningless (https://docs.python.org/3/tutorial/modules.html#intra-package-references). Besides, I suppose that prophyc/protoc are supposed to generate python modules, not packages (think: compiler (obj-file) vs linker (ELF) analogy in natively compiled languages).

I suppose it's not a good idea to make such restriction, especially since protoc doesn't seem to do that either. What could be done is to mimic the structure of source files in the structure of generated files. Now prophyc generates something like this:

(py36) aurzenligl@aurzenligl-pc ~/tmp/prophy $ tree src/
src/
├── subdir
│   ├── y.prophy
│   └── y.py
└── x.prophy

1 directory, 3 files
(py36) aurzenligl@aurzenligl-pc ~/tmp/prophy $ prophyc --python_out bld src/x.prophy src/subdir/y.prophy 
(py36) aurzenligl@aurzenligl-pc ~/tmp/prophy $ tree bld/
bld/
├── x.py
└── y.py

When protoc does something like this:

(py36) aurzenligl@aurzenligl-pc ~/tmp/proto $ tree src
src
├── subdir
│   └── y.proto
└── x.proto

1 directory, 2 files
(py36) aurzenligl@aurzenligl-pc ~/tmp/proto $ protoc -Isrc --python_out bld src/x.proto src/subdir/y.proto
(py36) aurzenligl@aurzenligl-pc ~/tmp/proto $ tree bld/
bld/
├── subdir
│   └── y_pb2.py
└── x_pb2.py
(py36) aurzenligl@aurzenligl-pc ~/tmp/proto $ cat bld/x_pb2.py | grep from | tail -1
from subdir import y_pb2 as subdir_dot_y__pb2

See this thread: https://github.com/protocolbuffers/protobuf/issues/1491#issuecomment-261621112

kamichal commented 5 years ago

As I read the protobuff issues story - their workaround does not work for us, because we do not care for the name of sources parent directory (or I missed something).

This assumes generated file would always be used as a part of package, otherwise from .(...) import would be meaningless.

If one file includes another - it means it really depends on symbols from imported file. Using single prophy_python.py separately is not meaningless it is impossible.

Other thing is that - in each generator - we output all files to single directory regardless its structure of source files. See: https://github.com/aurzenligl/prophy/blob/775ef4b7e7283b87d0e9e40be082cfcbe61c56fc/prophyc/generators/base.py#L37-L49

It comes from two assumptions:

Since that - relative imports proposed here:

It even gives more security: assume the input sys.cpp defines symbol named eggs used in knight.cpp. Currently it produces folowing content in knight.py:

# file knight.py
import prophy 

from sys import eggs
# or before v.1.2.0:
from sys import *

After accepting this MR it would become:

# file knight.py
import prophy
from .sys import eggs
florczakraf commented 4 years ago

@jqb1 are you going to finalize this MR somehow?

jqb1 commented 4 years ago

@florczakraf I guess the owners had a reason not to merge this one, I would need to have another look at this. Feel free to finish this if you want to.

florczakraf commented 4 years ago

@aurzenligl: the issue in protobuf you linked has been closed without a proper solution and it's very active to this day (especially now, when a lot of people are migrating their codebases to python3). It turns out that people tend to use the codecs in packages to have more control, not as top-level modules.The two most common solutions from the community are: use sed or 2to3. Both are quite impractical.

Besides, I suppose that prophyc/protoc are supposed to generate python modules, not packages

Perhaps we could introduce an option to let user decide whether we should generate modules or a package? And then, in second case the code proposed by @jqb1 does just the thing I believe.

aurzenligl commented 4 years ago

Sorry for silence -> let me revisit this case. @florczakraf or @jqb1 - can you generate a working example with a couple of .prophy files, script to generate .py codecs and script which attempts to use them in python3 which currently fails? I'd like to understand exact use case you'd like to enable.

It turns out that people tend to use the codecs in packages to have more control, not as top-level modules.

Sounds very reasonable to me, question is whether prophyc should generate those packages or should developer using prophyc put appropriate init.py files themselves.

The two most common solutions from the community are: use sed or 2to3. Both are quite impractical.

That sounds awful. Let's fix prophyc so that it generates proper output files for both python2 and python3.

aurzenligl commented 4 years ago

I think I know what you mean, I reproduced the issue here: https://github.com/aurzenligl/study/tree/master/prophy-relative-import

When run in py2:

(py27) aurzenligl@aurzbox:~/projects/study/prophy-relative-import$ ./build.sh && ./test.sh 
imported successfully

When run in py3:

(py36) aurzenligl@aurzbox:~/projects/study/prophy-relative-import$ ./build.sh && ./test.sh 
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/aurzenligl/projects/study/prophy-relative-import/packages/codec/d.py", line 6, in <module>
    from b import B
ModuleNotFoundError: No module named 'b'

When relative import is used, it works both in py2 and py3 - I'm not aware of any drawbacks.

aurzenligl commented 4 years ago

Closed in favor of #33