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

Feature/python bindings #905

Merged
merged 58 commits into from
Apr 22, 2019

Conversation

christian-krieg
Copy link

Python bindings to interact with the Yosys C++ API Yosys from within Python. This feature enables to call Yosys data structures and functions from a Python script, and to call passes written in Python from the Yosys shell.

Benedikt Tutzer added 30 commits June 22, 2018 11:15
…es/Modules can now move to a different parent without referencing issues
…and pointer match the pair saved in the corresponding map in kernel/rtlil.cc. Otherwise, the object was destroyed in c++ and should not be accessed any more
-IdString
-Const
-CaseRule
-SwitchRule
-SyncRule
-Process
-SigChunk
-SigBit
-SigSpec
With all their member functions as well as the remaining member
functions for Cell, Wire, Module and Design and static functions of
rtlil.h
…command. There are still issues when run in shell mode, but they can be used just fine in a python script
… managed by Python and destroyed when out of scope. Also, the file in which a function/struct was found is added to the comment before the function
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
@@ -261,6 +268,19 @@ ifeq ($(ENABLE_LIBYOSYS),1)
TARGETS += libyosys.so
endif

ifeq ($(ENABLE_PYOSYS),1)
ifeq ($(PYTHON_MAJOR_VERSION),3)
LDLIBS += -lpython$(PYTHON_VERSION)m -lboost_python-py$(subst .,,$(PYTHON_VERSION)) -lboost_system -lboost_filesystem

Choose a reason for hiding this comment

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

Arch Linux calls the Boost Python library boost_python37 rather than boost_python-py37. There is significant inter-distro variation here, see all the different cases that we ended up having to detect in nextpnr: https://github.com/YosysHQ/nextpnr/blob/master/CMakeLists.txt#L132-L186 - I'm not sure if there is a better way than bruteforcing all the possibilities here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the heads-up. I implemented 4 versions now:

  1. boost_python-py
  2. boost_python-py<major_version>
  3. boost_python
  4. boost_python<major_version>

If none of those work, the user is prompted to define BOOST_PYTHON_LIB manually.
cc270ea

Choose a reason for hiding this comment

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

Thanks, this works well for me now. One small comment is the auto-detection prints

/usr/bin/ld: cannot find -lboost_python-py37
collect2: error: ld returned 1 exit status
/usr/bin/ld: cannot find -lboost_python-py3
collect2: error: ld returned 1 exit status
/usr/bin/ld: cannot find -lboost_python-py37
collect2: error: ld returned 1 exit status
/usr/bin/ld: cannot find -lboost_python-py3
collect2: error: ld returned 1 exit status

at the moment, which could be confusing as it looks like an error. Perhaps you should redirect the output to /dev/null or similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suppressed error output in e19981a

Makefile Outdated Show resolved Hide resolved
@daveshah1
Copy link

Thanks for the reminder @cliffordwolf !

Generally very happy with this now. My last remaining issue is that as far as I can tell, this is enabled by default for all Yosys targets. It would be good to confirm that it works for all of them (e.g. mxe, msys2, Emscripten, Visual Studio) and if not, either fix it or disable it by default if it isn't practicable to get it to work (I suspect this may be the case for Emscripten at least).

@cliffordwolf cliffordwolf changed the title WIP: Feature/python bindings Feature/python bindings Apr 22, 2019
@cliffordwolf cliffordwolf merged commit 99d5435 into YosysHQ:master Apr 22, 2019
@cliffordwolf
Copy link
Collaborator

I've now merged it, but also set ENABLE_PYOSYS=0 as default value.

@cliffordwolf
Copy link
Collaborator

I'd like to keep the root dir as clean as possible. Would it be possible to move __init__.py and py_wrap_generator.py to misc/, maybe make __init__.py a make target and only place it in the top-level dir if we are actually performing a build with python enabled?

@btut
Copy link
Contributor

btut commented Apr 30, 2019

I'd like to keep the root dir as clean as possible. Would it be possible to move __init__.py and py_wrap_generator.py to misc/, maybe make __init__.py a make target and only place it in the top-level dir if we are actually performing a build with python enabled?

This is a simple fix. No need for a build-target for init.py, as it is not needed when importing the .so file from the yosys directory, just when it is installed, so copying it on install (from the misc dir) is enough.
I am not a github pro, is there any way to update this pull-request with a new commit?

My last remaining issue is that as far as I can tell, this is enabled by default for all Yosys targets. It would be good to confirm that it works for all of them

I need to setup a lot of virtual machines to test this out, unfortunately this will take some time.

@cliffordwolf
Copy link
Collaborator

I am not a github pro, is there any way to update this pull-request with a new commit?

No, but you can just push to your existing branch and create another PR. But usually one would create a new feature branch, forked from current master, and create a new PR. (This sounds like a hassle but it's really just a few seconds once you are used to git.)

I need to setup a lot of virtual machines to test this out, unfortunately this will take some time.

I did turn it off by default for now. So no need to get wild with testing on all platforms yet.

@btut
Copy link
Contributor

btut commented Apr 30, 2019

But usually one would create a new feature branch, forked from current master, and create a new PR.

I stuck to the same feature-branch for now since I already pushed to it before you commented, but will keep your suggestion in mind for next time. Thanks!

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.

4 participants