Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python binding and build enhancement #150

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wqh17101
Copy link
Contributor

add zbar_llite to support building python whl from any OS
add setup_plus.py to support building wheels with pre-compilered zbar lib
add ZBAR_LITE marco for future updates
add patch version to zbar python module

Signed-off-by: wqh17101 <597935261@qq.com>

(cherry picked from commit 49004fc)
Signed-off-by: wqh17101 <597935261@qq.com>
add .vs
Signed-off-by: wqh17101 <597935261@qq.com>
python/setup_plus.py Show resolved Hide resolved
python/zbar_init/__init__.py Show resolved Hide resolved
@mchehab
Copy link
Owner

mchehab commented Feb 12, 2021

Not sure what you're trying to achive with this patch series. Is it just due to build issues with Python bindings on Windows?

If so, the problem is very likely because PYTHON_LIBS was not set. In the case of Github's workflow, it should be initialized with:

PYTHON_LIBS= -LD:/a/_temp/msys/msys64/mingw64/lib -lpython3.8  -lm -lversion -lshlwapi -lm  

I have a patch fixing it, but it has a drawback: it causes MacOS build to fail.

@mchehab
Copy link
Owner

mchehab commented Feb 12, 2021

The patch which auto-detect the LDFLAGS is:

Author: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Date:   Fri Feb 12 17:08:13 2021 +0100

    configure.ac: parse python LDFLAGS
    
    At least on Windows, python bindings produce lots of linkedit
    errors, like:
    
            D:/a/_temp/msys/msys64/mingw32/lib/gcc/i686-w64-mingw32/10.2.0/../../../../i686-w64-mingw32/bin/ld.exe: python/.libs/zbar_la-zbarmodule.o: in function `Py_DECREF':
            D:/a/_temp/msys/msys64/mingw32/include/python3.8/object.h:478: undefined reference to `_Py_Dealloc'
            D:/a/_temp/msys/msys64/mingw32/lib/gcc/i686-w64-mingw32/10.2.0/../../../../i686-w64-mingw32/bin/ld.exe: python/.libs/zbar_la-zbarmodule.o: in function `increase_verbosity':
            D:\a\zbar\zbar/python/zbarmodule.c:173: undefined reference to `PyArg_ParseTuple'
    
    That's probably because the python libraries were not included.
    
    So, add a LDFLAGS to configure.
    
    Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

diff --git a/configure.ac b/configure.ac
index f1f0c84a97e6..6555f4e7c7c7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -560,7 +560,7 @@ Please notice that PYTHON var, if especified, takes precedence.])],
 
 AC_ARG_VAR([PYTHON_CONFIG], [full path to python-config program])
 AC_ARG_VAR([PYTHON_CFLAGS], [compiler flags for building python extensions])
-AC_ARG_VAR([PYTHON_LIBS], [linker flags for building python extensions])
+AC_ARG_VAR([PYTHON_LDFLAGS], [linker flags for building python extensions])
 
 AC_ARG_VAR([PYGTK_H2DEF], [full path to PyGTK h2def.py module (python2 only)])
 AC_ARG_VAR([PYGTK_CODEGEN], [full path to pygtk-codegen program (python2 only)])
@@ -593,7 +593,13 @@ AS_IF([test "x$PYTHON_VERSION" != "x" && test "x$with_python" != "xno"],
      [test -x "$PYTHON-config"],
      [PYTHON_CFLAGS=`$PYTHON-config --cflags`],
      [PYTHON_CFLAGS=`$PYTHON -c 'import distutils.sysconfig as s, sys; sys.stdout.write(" ".join(s.get_config_vars("CFLAGS")) + " -I"+s.get_python_inc() + " -I"+s.get_python_inc(plat_specific=True))'`])
-
+   AS_IF([test "x$PYTHON_LDFLAGS" != "x"],
+     [],
+     [test "x$PYTHON_CONFIG" != "x" && test -x "$PYTHON_CONFIG"],
+     [PYTHON_LDFLAGS=`$PYTHON_CONFIG --ldflags`],
+     [test -x "$PYTHON-config"],
+     [PYTHON_LDFLAGS=`$PYTHON-config --ldflags`],
+     [PYTHON_LDFLAGS=`$PYTHON -c 'import distutils.sysconfig as s, sys; sys.stdout.write(" ".join(s.get_config_vars("LDFLAGS")) + " -I"+s.get_python_inc() + " -I"+s.get_python_inc(plat_specific=True))'`])
    dnl check that #include <Python.h> compiles (bug #3092663)
    CPPFLAGS_save="$CPPFLAGS"
    CPPFLAGS="$CPPFLAGS $PYTHON_CFLAGS"
diff --git a/pygtk/Makefile.am.inc b/pygtk/Makefile.am.inc
index 9b34c3dfef69..033d601c4f0a 100644
--- a/pygtk/Makefile.am.inc
+++ b/pygtk/Makefile.am.inc
@@ -2,9 +2,9 @@ pyexec_LTLIBRARIES += pygtk/zbarpygtk.la
 pygtk_zbarpygtk_la_CPPFLAGS = \
     $(GTK_CFLAGS) $(PYTHON_CFLAGS) $(PYGTK_CFLAGS) $(AM_CPPFLAGS)
 pygtk_zbarpygtk_la_LDFLAGS = -shared -module -avoid-version -export-dynamic \
-    -export-symbols-regex initzbarpygtk
+     -export-symbols-regex initzbarpygtk $(PYTHON_LDFLAGS)
 pygtk_zbarpygtk_la_LIBADD = \
-    $(PYTHON_LIBS) $(PYGTK_LIBS) gtk/libzbargtk.la $(AM_LIBADD)
+    $(PYGTK_LIBS) gtk/libzbargtk.la $(AM_LIBADD)
 
 pygtk_zbarpygtk_la_DEPENDENCIES = gtk/libzbargtk.la
 dist_pygtk_zbarpygtk_la_SOURCES = pygtk/zbarpygtkmodule.c
diff --git a/python/Makefile.am.inc b/python/Makefile.am.inc
index f61214fe6488..ecc66df866a3 100644
--- a/python/Makefile.am.inc
+++ b/python/Makefile.am.inc
@@ -1,8 +1,9 @@
 pyexec_LTLIBRARIES += python/zbar.la
 python_zbar_la_CPPFLAGS = $(PYTHON_CFLAGS) $(AM_CPPFLAGS)
 python_zbar_la_LDFLAGS = -shared -module -avoid-version -export-dynamic \
-    -export-symbols-regex '(initzbar|PyInit_zbar)'
-python_zbar_la_LIBADD = $(PYTHON_LIBS) zbar/libzbar.la $(AM_LIBADD)
+    -export-symbols-regex '(initzbar|PyInit_zbar)' -no-undefined \
+    $(PYTHON_LDFLAGS)
+python_zbar_la_LIBADD = zbar/libzbar.la $(AM_LIBADD)
 
 python_zbar_la_SOURCES = python/zbarmodule.c python/zbarmodule.h \
     python/enum.c python/exception.c python/symbol.c python/symbolset.c \

@wqh17101
Copy link
Contributor Author

wqh17101 commented Feb 12, 2021

No, in the old version , the way python work with is to build a zbarlib and a python wrapper, there are two libs.
But for now using a pythonic way we can make a installer(wheel) with one lib contain above , it is easy for user to use without downloading the large source code to build.
So keep a original for some users, and provide a more convinient way is a good idea

Copy link
Owner

@mchehab mchehab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in the old version , the way python work with is to build a zbarlib and a python wrapper, there are two libs.
But for now using a pythonic way we can make a installer(wheel) with one lib contain above , it is
easy for user to use without downloading the large source code to build.

This is not a "large source code"... zbar/*.[ch] have 52k lines. The entire code has just 74k lines on C and 1,6k lines on python

:-)

Btw, the patch fixing configure.ac was applied. Python bindings are compiling fine for all targets.

So keep a original for some users, and provide a more convinient way tis a good idea

I'm ok on having a python binding that would be easer, provided that it is properly integrated with autotools. In any case, I suspect that some things could be simplified, now that the python libraries compile just fine on all OSes.

I guess I'll later add a make check to Windows and Mac OS actions CI workflow, in order to check if the built binaries are working properly there.

zbar_lite/README.md Show resolved Hide resolved
zbar_lite/README.md Show resolved Hide resolved
zbar_lite/config_template/linux/config.h Show resolved Hide resolved
zbar_lite/config_template/windows/config.h Show resolved Hide resolved
zbar_lite/preparation.sh Outdated Show resolved Hide resolved
zbar_lite/setup.py Outdated Show resolved Hide resolved
zbar_lite/setup.py Show resolved Hide resolved
zbar_lite/setup.py Show resolved Hide resolved
zbar_lite/setup.py Show resolved Hide resolved
zbar_lite/setup.py Show resolved Hide resolved
@wqh17101
Copy link
Contributor Author

Let me explain how ZBAR-LITE work.
run preparation.sh will copy the source code from ZBAR to a workdir named zbar_lite.
then if you want to install , run python setup.py install . It will intall a python package to your OS where other python packages in.
if you want to generate a python whl to release or just want to make a Plug-and-Play installer (.exe is also available too). you can run python setup.py bdist_wheel . It will build a whl to the build dir(defalut is zbar_lite/build). As setup.py ` s param , users can easily specify the path or other options.

@wqh17101
Copy link
Contributor Author

Threre are three ways for Python users to use zbar now.

  1. build and install libzbar and python zbar wrapper -- both the old one and setup_plus provide it .
  2. only build the python zbar wrapper, and provide a libzbar when using. --setup_plus provide it
    this two above for now is the python/setup_plus.py which is compatible with the old setup.py
    As a bakup i keep the old setup.py , if you think there is no need to keep it any more, i can remove it or just overwrite it with the content of setup_plus.py
  3. use zbar-lite which build only one lib with wrapper and libzbar -- zbar-lite provide it. or just use pip install zbar-lite to install from pypi.org which is the best way for python users.

@mchehab mchehab force-pushed the master branch 15 times, most recently from 8f60db5 to e97218f Compare February 13, 2021 15:26
@mchehab mchehab force-pushed the master branch 17 times, most recently from 658f548 to c7d9809 Compare February 14, 2021 19:56
@mchehab
Copy link
Owner

mchehab commented Feb 15, 2021

No, in the old version , the way python work with is to build a zbarlib and a python wrapper, there are two libs.
But for now using a pythonic way we can make a installer(wheel) with one lib contain above , it is easy for user to use without downloading the large source code to build.
So keep a original for some users, and provide a more convinient way is a good idea

Keeping the original and the new one as separate libraries is a bad idea, as, if someone compiles something against the libraries, there will be duplicated symbols. The best is to add the newer functions needed by the new python code at the same library (don't forget to increment library's version at configure.ac), and add a new configure.ac option to allow enabling it at compile time.

@mchehab
Copy link
Owner

mchehab commented Feb 15, 2021

Not sure what you're trying to achive with this patch series. Is it just due to build issues with Python bindings on Windows?

If so, the problem is very likely because PYTHON_LIBS was not set. In the case of Github's workflow, it should be initialized with:

PYTHON_LIBS= -LD:/a/_temp/msys/msys64/mingw64/lib -lpython3.8  -lm -lversion -lshlwapi -lm  

I have a patch fixing it, but it has a drawback: it causes MacOS build to fail.
Fixed. The problem is that Windows build required a different ld flag(-no-undefined):

   AS_IF([test "x$win32" = "xyes"], PYTHON_LDFLAGS="$PYTHON_LDFLAGS -no-undefined")

including below file:
config_template dir
MANIFEST.in
preparation.sh
README.md
setup.py
Signed-off-by: wqh17101 <597935261@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants