Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Copy abc stuff for pyosys to enable use of the abc pass #4901

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

akashlevy
Copy link
Contributor

@akashlevy akashlevy commented Feb 14, 2025

What are the reasons/motivation for this change?

Pyosys is incapable of using the abc pass without this change when using make install ENABLE_PYOSYS=1, as the binary is missing.

Explain how this is achieved.

Copy over yosys-abc binary and share directory to Pyosys install directory.

If applicable, please suggest to reviewers how they can test the change.

Try calling the abc pass in pyosys before and after the change when using make install ENABLE_PYOSYS=1 with a virtual environment

@KrystalDelusion
Copy link
Member

Has this broken since #4643? Or is that specific to building wheels, and if so does this break that?

@akashlevy
Copy link
Contributor Author

That is specific to building wheels. This fixes the same issue for make install target when using a Python virtual environment or installing directly to site packages

@KrystalDelusion
Copy link
Member

Okay, seems fine to me then. @mmicko are you familiar with how the wheels are built? Does this seem likely to break anything there or is it good to merge?

@mmicko
Copy link
Member

mmicko commented Feb 18, 2025

@donn could you please check if this is affecting wheels build ?

@donn
Copy link
Contributor

donn commented Feb 18, 2025

@mmicko I'll test, but…


@akashlevy The wheels should already include both yosys-abc and share. How are you building them?

yosys/setup.py

Line 76 in 38f8583

# yosys-abc

@akashlevy
Copy link
Contributor Author

I'm building with make install instead of building a wheel and loading that. I figure this should be a supported installation method as well

@donn
Copy link
Contributor

donn commented Feb 18, 2025

make install ENABLE_PYOSYS=1 should install both yosys-abc and the Yosys share directory already is the thing. Wheels are kind of a special case. If import libyosys is not finding the globally installed yosys-abc and Yosys share, that's its own issue…

@akashlevy
Copy link
Contributor Author

akashlevy commented Feb 18, 2025

Yes, I guess the real issue is that the search path does not get configured correctly in the general case (which was what #4643 was attempting to fix I guess?)

For example, the workflow I use (which is pretty standard for Python development AFAIK) is to have a Python virtual environment called .venv and use make install to build the package into that. In this scenario, the current setup does not allow libyosys to properly find yosys-abc and the share directory.

The best solution I could see is to just copy these resources right into the pyosys directory. That's guaranteed to work as that's the first place libyosys looks for them. I'm open to alternatives though

@akashlevy
Copy link
Contributor Author

akashlevy commented Feb 18, 2025

If we think it makes most sense to use the wheel, then perhaps we should use a wheel-style installation for make install when pyosys is enabled? Rather than copying stuff

@donn
Copy link
Contributor

donn commented Feb 18, 2025

Even if you installed it into a venv, as long as you set PREFIX correctly for make build, the binary will automatically fall back to PREFIX/share/yosys when it doesn't find a dedicated yosys-abc or share directory in the pyosys directory:

# ifdef YOSYS_DATDIR

The fact that's not working suggests a deeper issue. Would you kindly share what installation commands you're running exactly? Just make install ENABLE_PYOSYS=1 would not actually install it into a venv, as venvs do not set PREFIX.

(Also, the conventional way to install something into a venv is still pip3 install .…)


At any rate, the wheels still work great with this PR.

@akashlevy
Copy link
Contributor Author

Here's what I have been using:

cd third_party/yosys && 
$(MAKE) install DESTDIR=../../.venv/ PREFIX= \
PYTHON_DESTDIR=`find ../../.venv/lib/python* -name "site-packages" | cut -c 13-` \
ENABLE_PYOSYS=1

Honestly, now that I look at this again, it does feel quite roundabout. I think it makes way more sense to just do pip3 install .

I propose we remove the legacy make install stuff that was manually copying pyosys into the Python site packages, and replace it with a wheel build when Pyosys is enabled. Does this seem reasonable?

@KrystalDelusion
Copy link
Member

I propose we remove the legacy make install stuff that was manually copying pyosys into the Python site packages, and replace it with a wheel build when Pyosys is enabled. Does this seem reasonable?

That seems like the better approach here.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants