Created attachment 213575 [details] [review]
fix python's configure checks

Review of attachment 213574 [details] [review]:

This is not correct as the python interpreter is statically linked to libpython and linking the gstreamer module to the library will caused the library to be in memory twice when python is used from the interpreter (in contrast to be used from inside an application).

If anything this should be optional for windows only.

Created attachment 213648 [details] [review]
fix compilation in windows

Updated patch to link conditionally with PYTHON_LIBS only on windows

Review of attachment 213575 [details] [review]:

::: configure.ac
@@ +99,3 @@
+if test "x$platform_win32" = "xyes" ; then
+  # skip python cheks if compiling to windows
+  # $PYTHON_INCLUDES and PYTHON_LIBS should be set properly

Why can't you use AM_CHECK_PYTHON_HEADERS() or an equivalent on Windows? Requiring the user to properly set this seems rather unintuitive

@@ +454,1 @@
 

Why do you remove the valgrind check?

Comment on attachment 213648 [details] [review]
fix compilation in windows

Looks good to me but depends on the other patch (Could you split the other into two chunks maybe, one for setting PLATFORM_WIN32?)

This macro has several issues that makes it impossible to use for windows.

The first problem comes when this macro is being used in a cross-compilation environment. This macro calls the native python to guess -I -L and -l which is wrong as you will get the details for the system's python and not the target one. Also this macro don't check whether PYTHON_INCLUDES or PYTHON_LIBS is set to skip the check.
On a native environment, this macro makes use of python-config, which is not available on Windows

if $PYTHON-config --help 2>/dev/null; then
   PYTHON_INCLUDES=`$PYTHON-config --includes 2>/dev/null`
else
   PYTHON_INCLUDES="-I${py_prefix}/include/python${PYTHON_VERSION}"

The fallback path is wrong as the windows installer installs the python headers in ${py_prefix}/Include


For PYTHON_LIBS -L is also wrong because libpython27.dll, which is the dll used for linking, is not in ${py_prefix}/lib but in c:/Windows/System32

if $PYTHON-config --help 2>/dev/null; then
 PYTHON_LIBS=`$PYTHON-config --ldflags 2>/dev/null`
 PYTHON_LIB=`$PYTHON -c "import distutils.sysconfig as s; print s.get_python_lib(standard_lib=1)"`
[...]
else
 PYTHON_LIBS="-L${py_prefix}/lib -lpython${PYTHON_VERSION}"
 PYTHON_LIB_LOC="${py_prefix}/lib"
fi


Also py_prefix must be overridden with the am_cv_python_pyexecdir and am_cv_python_pythondir because paths from the python command are windows paths with backslashes which are escaped.

The check is defined in acinclude.m4 so it could be fixed there instead.

Yes, should be fixed in acinclude.m4 and where the macro comes from (Python maybe?)

(In reply to comment #6)
> This macro has several issues that makes it impossible to use for windows.
> 
> The first problem comes when this macro is being used in a cross-compilation
> environment. This macro calls the native python to guess -I -L and -l which is
> wrong as you will get the details for the system's python and not the target
> one.

Ack, for cross compilation it shouldn't call any native binaries.

> Also this macro don't check whether PYTHON_INCLUDES or PYTHON_LIBS is set
> to skip the check.

Yes, skipping the test if the variables are set would be the correct behaviour

> On a native environment, this macro makes use of python-config, which is not
> available on Windows
> 
> if $PYTHON-config --help 2>/dev/null; then
>    PYTHON_INCLUDES=`$PYTHON-config --includes 2>/dev/null`
> else
>    PYTHON_INCLUDES="-I${py_prefix}/include/python${PYTHON_VERSION}"
> 
> The fallback path is wrong as the windows installer installs the python headers
> in ${py_prefix}/Include

So platform specific fallbacks here

> For PYTHON_LIBS -L is also wrong because libpython27.dll, which is the dll used
> for linking, is not in ${py_prefix}/lib but in c:/Windows/System32
> 
> if $PYTHON-config --help 2>/dev/null; then
>  PYTHON_LIBS=`$PYTHON-config --ldflags 2>/dev/null`
>  PYTHON_LIB=`$PYTHON -c "import distutils.sysconfig as s; print
> s.get_python_lib(standard_lib=1)"`
> [...]
> else
>  PYTHON_LIBS="-L${py_prefix}/lib -lpython${PYTHON_VERSION}"
>  PYTHON_LIB_LOC="${py_prefix}/lib"
> fi

... and here

> Also py_prefix must be overridden with the am_cv_python_pyexecdir and
> am_cv_python_pythondir because paths from the python command are windows paths
> with backslashes which are escaped.

Ok

What should we do with this? gst-python is completely different in master, is this still relevant or necessary?

ping. There are patches lying around.

Andoni: is this still relevant with 1.x gst-python, or can this be closed?

I think it's not needed anymore