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

fix building with python2 and python3 #483

Merged
merged 2 commits into from
Oct 10, 2019

Conversation

mikhailnov
Copy link
Contributor

@mikhailnov mikhailnov commented Oct 10, 2019

I wanted to build mlt with both python2 and python3 for ROSA Linux and had to patch like this to allow this to be done.
https://abf.io/import/mlt/commit/eff21006ee45d596cf4456ff2469f51f5d55cde9

In ROSA 2016.1 it is /usr/include/python3.5m and build script tried to find /usr/include/python3.5
Use python-config to get includes
@ddennedy ddennedy merged commit c7ca482 into mltframework:master Oct 10, 2019
@ddennedy
Copy link
Member

I hope it works well enough for all.

@ddennedy ddennedy added this to the v6.18.0 milestone Oct 10, 2019
@sergiomb2
Copy link
Contributor

sergiomb2 commented Nov 5, 2019

Hi,
I have another suggestion which I think is better, the line at the end [1]

[1]

export PYTHON_INCLUDE=`"$PYTHON" -c "from sysconfig import get_paths; print(get_paths()['include'])"`

@ddennedy
Copy link
Member

ddennedy commented Nov 7, 2019

@sergiomb2 why? Is python[23]-config --includes broken on your distro? Can you cite official documentation to support your claim?

@mikhailnov
Copy link
Contributor Author

mikhailnov commented Nov 7, 2019 via email

@sergiomb2
Copy link
Contributor

here [1] is the problem when PYTHON_INCLUDE=-I/usr/local/include/python3.6m -I/usr/local/include/python3.6m .
Notes: ubuntu is bronken and I use "${PYTHON}-config" --ldflags .
But since we are talking I propose copy python to python3 and have 2 directories like I do in Fedora [2]

[1]
-c -I../.. -I$PYTHON_INCLUDE

[2]
https://src.fedoraproject.org/rpms/mlt/blob/master/f/mlt.spec#_174

@sergiomb2
Copy link
Contributor

note that all swig/python/*.py have she bangs to one python version , for example [1]

[1]
https://github.com/mltframework/mlt/blob/master/src/swig/python/codecs.py#L1

@mikhailnov
Copy link
Contributor Author

mikhailnov commented Nov 7, 2019

07.11.2019 12:28, Sérgio Basto пишет:

here [1] is the problem when PYTHON_INCLUDE=-I/usr/local/include/python3.6m -I/usr/local/include/python3.6m .

I don't understand what the problem is. How do you get /usr/local when building a distro package in Fedora?!

Notes: ubuntu is bronken and I use "${PYTHON}-config" --ldflags .
But since we are talking I propose copy python to python3 and have 2 directories like I do in Fedora [2]

[1]
-c -I../.. -I$PYTHON_INCLUDE

[2]
https://src.fedoraproject.org/rpms/mlt/blob/master/f/mlt.spec#_174

That is a hack. ( cd src/swig/python3; export PYTHON=python2; ./build ) how I do in ROSA (https://abf.io/import/mlt/blob/rosa2019.1/mlt.spec#lc-211) is a much shorter and because of this more reliable solution than yours in Fedora, at least I believe so.

note that all swig/python/*.py have she bangs to one python version , for example [1]

Good catch... We should fix it

@mikhailnov
Copy link
Contributor Author

Shebang is not a problem because it does not exist in mlt.py which is the only *.py file being installed. But I had already written a Makefile capable of fixing shebangs before I noticed that shebang is not a problem, sent it in #495

@sergiomb2
Copy link
Contributor

my propose #496

@ddennedy
Copy link
Member

ddennedy commented Nov 7, 2019

But since we are talking I propose copy python to python3 and have 2 directories like I do in Fedora [2]

No, python2 is no longer supported. Now, I regret accepting the change that restores support for it in MLT. if you need it in your distro, patch the source for your package and do not try to upstream it - at least for this package.

note that all swig/python/*.py have she bangs to one python version
Good catch... We should fix it

No, we should not. If you want to use the example python script with python 2.x, then invoke it with that interpreter or edit the example.

-I$PYTHON_INCLUDE is obviously a problem since the output of python3-config --includes includes the -I flags.

@sergiomb2
Copy link
Contributor

OK , no problem , we in Fedora + RPMFusion have one python-mlt client which is Flowblade .
Flowable will have an python3 version before end of the year [1] , and that is why I'd like have python2 and python3 version to migration be more smooth .

About -I$PYTHON_INCLUDE seems to me we just need remove -I and use just $PYTHON_INCLUDE

Thank you

[1]
jliljebl/flowblade#597

@ddennedy
Copy link
Member

ddennedy commented Nov 7, 2019

About -I$PYTHON_INCLUDE seems to me we just need remove -I and use just $PYTHON_INCLUDE

Yes, I made that change.

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.

None yet

3 participants