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

build one python binding only #109

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

waebbl
Copy link
Contributor

@waebbl waebbl commented Mar 5, 2021

I tried to look into that. The patch worked for me with python-3, but unfortunately I couldn't test with python-2.
Feel free to review and address any changes needed, if you want to consider the patch.

Bug: #108
Signed-off-by: Bernd Waibel waebbl-gentoo@posteo.net

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

This looks good to me. Would be good to get a second reviewer.

@cary-ilm
Copy link
Member

cary-ilm commented Mar 7, 2021

Thank you for this! I tried it out and it seems to work just as expected. I'm glad to have this in place, it simplifies matters.

Wouldn't it be better for the option to be USE_PYTHON2 and default to off, rather than USE_PYTHON3 and default to on? That way, saying nothing about python version would do the right modern thing, and when we drop support for python2, it's just a matter of removing the option altogether.

@waebbl
Copy link
Contributor Author

waebbl commented Mar 8, 2021

Wouldn't it be better for the option to be USE_PYTHON2 and default to off, rather than USE_PYTHON3 and default to on? That way, saying nothing about python version would do the right modern thing, and when we drop support for python2, it's just a matter of removing the option altogether.

I'm open for both ways.
Though I wonder if it makes a difference at all?

  • Current way: py3 default to ON, so saying nothing will do the modern thing, if py2 is dropped, only -DPYTHON is needed and it can be dropped.
  • Alternative way: py2 default to OFF, saying nothing will do the modern thing, if py2 is dropped, only -DPYTHON is needed and it can be dropped.

Or do I miss something?

@cary-ilm
Copy link
Member

cary-ilm commented Mar 8, 2021 via email

@waebbl
Copy link
Contributor Author

waebbl commented Mar 8, 2021

I understand. Gonna change it to USE_PYTHON2 then.

src/python/CMakeLists.txt Outdated Show resolved Hide resolved
@cary-ilm
Copy link
Member

cary-ilm commented Mar 9, 2021

I downloaded this and tried it out on both linux and macOS systems that have both python2 and python3 and it seems to work as expected. Can someone also confirm this does the right thing on Windows?

If you can delete the commented-out lines, we should be good to go. Thanks!

@waebbl
Copy link
Contributor Author

waebbl commented Mar 9, 2021

Thanks for testing this on alternate platforms and python2.
I removed the comments and just noticed I have to pull changes first, so there'll be another push soon.

Bug: AcademySoftwareFoundation#108
Signed-off-by: Bernd Waibel <waebbl-gentoo@posteo.net>
@cary-ilm cary-ilm merged commit e1bbea7 into AcademySoftwareFoundation:master Mar 9, 2021
@waebbl waebbl deleted the cmake-python branch March 9, 2021 17:14
@cary-ilm cary-ilm added the build label Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants