After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 691082 - Support py2 and py3 loader in the build
Support py2 and py3 loader in the build
Status: RESOLVED FIXED
Product: libpeas
Classification: Platform
Component: python
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libpeas-maint
libpeas-maint
: 690350 691128 (view as bug list)
Depends on:
Blocks: python3
 
 
Reported: 2013-01-03 17:16 UTC by Paolo Borelli
Modified: 2013-01-05 17:49 UTC
See Also:
GNOME target: 3.8
GNOME version: 3.7/3.8


Attachments
patch (10.16 KB, patch)
2013-01-03 17:16 UTC, Paolo Borelli
none Details | Review
patch (19.95 KB, patch)
2013-01-03 18:25 UTC, Paolo Borelli
none Details | Review
patch (20.70 KB, patch)
2013-01-03 18:59 UTC, Paolo Borelli
none Details | Review
patch (26.28 KB, patch)
2013-01-03 20:34 UTC, Paolo Borelli
none Details | Review
patch (27.38 KB, patch)
2013-01-03 21:21 UTC, Paolo Borelli
none Details | Review
prelim patch (962 bytes, patch)
2013-01-05 00:19 UTC, Paolo Borelli
none Details | Review
patch (27.65 KB, patch)
2013-01-05 00:19 UTC, Paolo Borelli
none Details | Review
prelim patch (1.46 KB, patch)
2013-01-05 00:26 UTC, Paolo Borelli
accepted-commit_now Details | Review
patch (27.65 KB, patch)
2013-01-05 00:27 UTC, Paolo Borelli
none Details | Review
patch (27.61 KB, patch)
2013-01-05 10:50 UTC, Paolo Borelli
none Details | Review
patch (27.62 KB, patch)
2013-01-05 17:23 UTC, Paolo Borelli
none Details | Review

Description Paolo Borelli 2013-01-03 17:16:21 UTC
We need to build and install both loaders since some apps will use one and some the other
Comment 1 Paolo Borelli 2013-01-03 17:16:54 UTC
Created attachment 232653 [details] [review]
patch
Comment 2 Paolo Borelli 2013-01-03 17:20:24 UTC
*** Bug 690350 has been marked as a duplicate of this bug. ***
Comment 3 Paolo Borelli 2013-01-03 17:21:37 UTC
Note: I have not tested yet, but I was happy enough to see the loader built and installed
Comment 4 Paolo Borelli 2013-01-03 18:25:48 UTC
Created attachment 232658 [details] [review]
patch

Updated patch, now make check passes.

Note: I had to rename the testcase plugin files to "python3"... Maybe we can instead copy them and keep both py2 and py3 test? That would mean duplicate files, but I guess for tests it would be ok...
Comment 5 Paolo Borelli 2013-01-03 18:59:27 UTC
Created attachment 232659 [details] [review]
patch

Improved patch, it now generates both py2 and py3 test with the same trick of gjs/seed
Comment 6 Garrett Regier 2013-01-03 19:28:47 UTC
Review of attachment 232659 [details] [review]:

::: configure.ac
@@ +325,3 @@
+    [enable_python2=auto])
+
+found_python2=no

should keep the old message "no (disabled, use ...)"

@@ +331,3 @@
+    AC_CHECK_PROGS(PYTHON2, [python2 python2.7 python2.6 python2.5])
+    if test -z "${PYTHON2}"; then
+found_python2=no

should not error if enable_python2=auto

@@ +336,3 @@
+    AC_PATH_TOOL(PYTHON2_CONFIG, "python2-config")
+    if test -z "${PYTHON2_CONFIG}"; then
+    dnl NOTE: we do not use AM_PATH_PYTHON, since it does not allow

same

@@ +362,3 @@
+    AC_PATH_TOOL(PYTHON3_CONFIG, "python3-config")
+    if test -z "${PYTHON3_CONFIG}"; then
+found_python3=no

should not error if enable_python3=auto

::: loaders/python/Makefile.am
@@ +11,3 @@
+	$(PYGOBJECT_CFLAGS)		\
+	$(PYTHON2_CFLAGS)		\
+	-DPEAS_PYEXECDIR=\""$(PYTHON_PYEXECDIR)"\" \

should be PYTHON2_PYEXECDIR

::: tests/libpeas/Makefile.am
@@ +43,2 @@
 endif
 

Would be nice to have python 2 tests as well

::: tests/libpeas/extension-python.c
@@ +119,2 @@
   EXTENSION_TEST (python, "instance-refcount", instance_refcount);
   EXTENSION_TEST (python, "activatable-subject-refcount", activatable_subject_refcount);

these should also be changed to python3

::: tests/libpeas/plugins/extension-python/Makefile.am
@@ +1,3 @@
 include $(top_srcdir)/tests/Makefile.plugin
 
+noinst_DATA = \

changing from noinst_PLUGIN to noinst_DATA will not work. noinst_PLUGIN is special see the included makefile

@@ +25,3 @@
+EXTRA_DIST = \
+	extension-py.gschema.xml	\
+	$(SED) -i -e 's%PY_LOADER%$(@:extension-%.gschema.xml=%)%g' $@

should not be .js

::: tests/libpeas/plugins/extension-python3-nonexistent.plugin
@@ +1,2 @@
+[Plugin]
+Module=extension-python-nonexistent

python3

@@ +2,3 @@
+Module=extension-python-nonexistent
+Loader=python3
+Name=Extension Python Nonexistent

also python3
Comment 7 Paolo Borelli 2013-01-03 20:34:37 UTC
Created attachment 232670 [details] [review]
patch

Ok, now it should be ready. With respect to the previous version:
 - fix the py.js typo in the dist files
 - fix tests so that both py2 and py3 unit tests are executed
Comment 8 Paolo Borelli 2013-01-03 21:21:56 UTC
Created attachment 232671 [details] [review]
patch

This addresses all the issues pointed out by Garrett
Comment 9 Javier Jardón (IRC: jjardon) 2013-01-04 12:54:37 UTC
*** Bug 691128 has been marked as a duplicate of this bug. ***
Comment 10 Paolo Borelli 2013-01-05 00:19:07 UTC
Created attachment 232795 [details] [review]
prelim patch

This preliminary patch prevents creating pyc files during tests, and makes our life easier
Comment 11 Paolo Borelli 2013-01-05 00:19:57 UTC
Created attachment 232796 [details] [review]
patch

Updated patch: it now passes distcheck
Comment 12 Paolo Borelli 2013-01-05 00:26:54 UTC
Created attachment 232797 [details] [review]
prelim patch

Preliminary patch that disables generating pyc
Comment 13 Paolo Borelli 2013-01-05 00:27:40 UTC
Created attachment 232798 [details] [review]
patch

Patch to support py2 and py3. Passes distcheck
Comment 14 Garrett Regier 2013-01-05 01:05:13 UTC
Review of attachment 232798 [details] [review]:

::: .gitignore
@@ +92,3 @@
+/tests/libpeas/plugins/extension-python/extension-python3.gschema.xml
+/tests/libpeas/plugins/extension-python/extension-python3.py
+/tests/libpeas/plugins/extension-python/extension-python3.plugin

sort .plugin before .py

::: configure.ac
@@ +357,3 @@
+                ])
+            ], [
+                    PYTHON2_CFLAGS=`${PYTHON2_CONFIG} --includes`

should be config not found

@@ +360,3 @@
+            ])
+        else
+                    pyexecdir=`$PYTHON2 -c "import sys; from distutils import sysconfig; print sysconfig.get_python_lib(True, prefix='')"`

should be version too old

@@ +366,3 @@
 
+if test "x$enable_python2" = "xyes" -a "x$found_python2" != "xyes"; then
+    AC_MSG_ERROR([You need to have Python 2 and PyGobject installed to build libpeas])

PyGObject

@@ +408,2 @@
+if test "x$enable_python3" = "xyes" -a "x$found_python3" != "xyes"; then
+    AC_MSG_ERROR([You need to have Python 3 and PyGobject installed to build libpeas])

PyGObject

::: loaders/python3/Makefile.am
@@ +1,1 @@
+# Python plugin loader

Python 3 plugin loader

@@ +31,3 @@
+	$(PYTHON3_LIBS)
+
+gcov_sources =	$(libpython3loader_la_SOURCES)

weird spacing after =

::: tests/libpeas/Makefile.am
@@ +47,3 @@
+extension_python3_SOURCES = extension-python3.c
+extension_python3_CFLAGS  = $(PYGOBJECT_CFLAGS) $(PYTHON3_CFLAGS)
+

needs extra space after variable for correct alignment

::: tests/libpeas/plugins/extension-python/Makefile.am
@@ +31,2 @@
+clean-local:
+	@rm -f gschemas.compiled;

If we are going to remove gschemas.compiled like this then we should remove it from Makefile.plugin and add the same rule to extension-{c,js}/Makefile
Comment 15 Garrett Regier 2013-01-05 01:06:53 UTC
Review of attachment 232797 [details] [review]:

looks good
Comment 16 Paolo Borelli 2013-01-05 10:48:56 UTC
Review of attachment 232798 [details] [review]:

::: tests/libpeas/plugins/extension-python/Makefile.am
@@ +31,2 @@
+clean-local:
+	@rm -f gschemas.compiled;

I will just go back to pulling in Makefile.plugin even if I do not use noinst_PLUGIN. We can decide if we want to keep Makefile.plugin or not in an unrelated patch
Comment 17 Paolo Borelli 2013-01-05 10:50:37 UTC
Created attachment 232816 [details] [review]
patch

Fix the issues pointed out in the review
Comment 18 Paolo Borelli 2013-01-05 17:23:09 UTC
Created attachment 232830 [details] [review]
patch

Fixed a problem spotted by Garrett
Comment 19 Paolo Borelli 2013-01-05 17:49:31 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.