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

[akashlevy]

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]

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

[akashlevy]

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]

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]

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

[donn]

@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]

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]

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]

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]

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]

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:

yosys/kernel/yosys.cc

Line 934 in 38f8583

# 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]

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]

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.

[akashlevy]

@donn @mmicko @KrystalDelusion see now for approach using wheel

[KrystalDelusion]

Since the wheel/install-wheel targets are always available, it might be nice to automatically set ENABLE_PYOSYS if the user has explicitly called make install-wheel? That or put those two targets inside a ifeq ($(ENABLE_PYOSYS),1) block.

[akashlevy]

I don't know how to set a variable from within a target (is that even possible?)

Instead, I used the ifeq ($(ENABLE_PYOSYS),1) approach and throw an error if ENABLE_PYOSYS is not set with the wheel or install_wheel targets.

I also added a few more things to clean target that were missing for pyosys

[KrystalDelusion]

I don't know how to set a variable from within a target (is that even possible?)

I don't think you can set it within a target, and even if you did it would be too late to actually do what it needs to. I think it is possible to check the target, but yeah the other way is much easier, and nice that you put the error on it too :)

[akashlevy]

Thanks :)