Port upstream Shapely 2.0.1 #1
Conversation
README.md
Outdated
| Shapely Python extension for Unikraft | ||
| ===================================== | ||
|
|
||
| This is a port of the [Shapely](https://pypi.org/project/shapely/) Python | ||
| extension to Unikraft. | ||
|
|
||
| Please refer to the `README.md` as well as the documentation in the | ||
| `doc/` subdirectory of the main unikraft repository for further | ||
| information. |
There was a problem hiding this comment.
Change this to md format instead of rst.
Use one line per sentence and one sentence per line.
Config.uk
Outdated
| config LIBPYTHON_SHAPELY | ||
| bool "Shapely - Python package for planar geometric objects" | ||
| default n | ||
| select LIBPYTHON_NUMPY | ||
| select LIBGEOS |
There was a problem hiding this comment.
I would add select X for all the necessary dependencies, it would be difficult to keep track of them otherwise.
There was a problem hiding this comment.
I believe the one direct dependency that's missing is python3, as shapely directly makes use of python API headers; I'll add it to the list.
As for other, indirect dependencies, I strongly believe it's better to delegate to the depended-on libraries instead of aggregating duplicate information here.
| touch $@ | ||
|
|
||
| # Add shapely rootfs to main python | ||
| python-rootfs: $(PYTHON_ROOTFS)/.shapely_done |
There was a problem hiding this comment.
Who will configure PYTHON_ROOTFS? Will it be done by lib-python3?
There was a problem hiding this comment.
Yes, PYTHON_ROOTFS is set when the python-rootfs make target is being run and taken from whatever you specify on the cmdline under path=, with the logic being provided by lib-python3.
If lib-python3 is not present/selected this line becomes a no-op, as no recipe for python-rootfs exists, nor is it an implicit target of anything.
Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Files under `generated/` are created by the upstream build process from templates; we include these files verbatim for the unikraft build. Any potential patches should be clearly marked to ease future updates. Files under `importfix/` are correctly namespaced wrapper modules for any binary modules that we compile and link into unikraft, and must be copied over to the python rootfs. Selecting this library will add its files to the python rootfs build. Signed-off-by: Andrei Tatar <andrei@unikraft.io>
There was a problem hiding this comment.
Looks very good, @andreittr, thank you for that. I left two minor questions regarding python, they should be easy to deal with.
| @@ -0,0 +1,9 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Here we should have python or python3?
There was a problem hiding this comment.
Depends; most distros today have python symlinked to python3 (as IMO one should, py2 is very deprecated), with scarce few outliers remaining.
On unikraft however it is irrelevant since the hashbang line is never taken into account (we don't have #! support, nor even exec()), and even if it were we have python -> python3 in the standard rootfs.
| @@ -0,0 +1,9 @@ | |||
| #!/usr/bin/env python | |||
|
|
|||
| from shapely__geometry_helpers import * | |||
There was a problem hiding this comment.
It is the * import actually useful, since we import from the same module a line below?
There was a problem hiding this comment.
Very much so, the * imports everything present in the module's __all__ attribute, while the following line explicitly imports things that aren't in __all__.
This is necessary so that the fully-namespaced pure python module (shapely._geometry_helpers) behaves exactly the same as the built-in compiled module it masquerades as, so no calling code can tell the difference.
There was a problem hiding this comment.
All good @andreittr, I'll add the tag after all numpy and libgeos dependencies are merged.
There was a problem hiding this comment.
All good with me. Many thanks again @andreittr for the consistent effort!!
Reviewed-by: Radu Nichita radunichita99@gmail.com
There was a problem hiding this comment.
@andreittr there seems to be a problem with the installation of the shapely module.
In the end it is not copied in the PYTHON_ROOTFS dir. It looks like the pip install . --prefix=$(PYTHON_ROOTFS) --no-compile command fails with
x86_64-linux-gnu-gcc -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -fPIC -I/usr/include/python3.11 -I/tmp/pip-build-env-dslda_80/overlay/local/lib/python3.11/dist-packages/numpy/core/include -c src/c_api.c -o build/temp.linux-x86_64-cpython-311/src/c_api.o
In file included from src/c_api.h:22,
from src/c_api.c:15:
src/geos.h:15:10: fatal error: geos_c.h: No such file or directory
15 | #include <geos_c.h>
I've been looking into it with @mariasfiraiala and that is likely the cause.
Aw crap, I see what's going wrong: pip is looking for shapely's build deps on the host system (which I incidentally had left over from the porting job so I didn't bump into it). Nice catch. |
This patch to setup.py prevents pip from trying to compile the C/C++ shapely sources when building the Unikraft Python rootfs, since we never make use of these compiled files. This prevents build failures when the host system lacks dev files for shapely dependencies. Signed-off-by: Andrei Tatar <andrei@unikraft.io>
|
Should be fixed now; I've patched setup.py to not build these files. As a bonus now rootfs building is much faster for shapely 😄 ; semi-offtopic: would've been nice to have smth like this for numpy but it's not as trivial, some other time. |
|
I was able to get shapely working by creating a custom rootfs, @andreittr! Should the shapely module be also put in the rootfs for the python3 app? |
|
@andreittr I tried this with the last patch, and it still does not seem to add the LE: nvm, I missed the Few issues:
This is anyway dev stuff, from a user point of view, as @RaduNichita said, I think we should have everything in the |
Nice! I assume you used the standard rootfs recipe after building a unikraft image with shapely (if not, do try that out, it's by far the easiest).
I doubt that's the right place to put it, since
As you mention, there's the
It is fairly well documented in
Indeed, and that's precisely why you get a warning if you try to build a rootfs with a python version that does not match Unikraft python; it also completely errors out if you try to build pyc files with the wrong python version too.
I think that is a bad call considering the purpose of I am ok with creating "demo apps" (e.g., |
Ok, fair. I think "demo apps" are a good idea. The main point is to have users able to use the stuff that we provide without too much work on their side, and I think having apps for python extensions will achieve that nicely. Not adding the extensions in the python app or having a separate
If we go for separate applications that's not needed, we can have the python3 application as it is. And one more question, why is the |
Oh crap, I did not realize that was the case, it most definitely should not be there; this is a bug, thanks for catching it. Looking into fixing it right now. |
Do you need something special for that? I tried running So, to make it work this PR, I had to move the site-packages from the |
Got it, then this is fine, we can merge it. Ee'll wait for the pr on |
There was a problem hiding this comment.
Since this is finally working and doesn't need any more amends, shall we merge it?
Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com
There was a problem hiding this comment.
Ok, so let's merge as it is and focus on the demo appa
Reviewed-by: Radu Nichita radunichita99@gmail.com
Did you receive the warning that your host Python is a different version? If so, then it's normal to get both dirs under site-packages/, you can use a virtualenv with the right python version to prevent this. |
|
@StefanJum @mariasfiraiala @RaduNichita Wait, let's not merge yet; the issue with numpy wrongly being in the rootfs has to do with these lib PRs. |
Yes, I agree, but let's document this as well. |
How so? The |
But the fact that it's there in the tar archive to begin with suggests there might be a bug in numpy's |
I see, we'll wait then. |
|
Ok, double-checked, rebuilt rootfs and it seems ok; I think I just messed up before. Rootfs fix in this PR: Other than that, all clear for merge. |
Signed-off-by: Andrei Tatar <andrei@unikraft.io> Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com> Reviewed-by: Radu Nichita <radunichita99@gmail.com> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1
Files under `generated/` are created by the upstream build process from templates; we include these files verbatim for the unikraft build. Any potential patches should be clearly marked to ease future updates. Files under `importfix/` are correctly namespaced wrapper modules for any binary modules that we compile and link into unikraft, and must be copied over to the python rootfs. Selecting this library will add its files to the python rootfs build. Signed-off-by: Andrei Tatar <andrei@unikraft.io> Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com> Reviewed-by: Radu Nichita <radunichita99@gmail.com> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1
This patch to setup.py prevents pip from trying to compile the C/C++ shapely sources when building the Unikraft Python rootfs, since we never make use of these compiled files. This prevents build failures when the host system lacks dev files for shapely dependencies. Signed-off-by: Andrei Tatar <andrei@unikraft.io> Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com> Reviewed-by: Radu Nichita <radunichita99@gmail.com> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Closes: #1
This change set ports the Shapely Python extension to unikraft at upstream version 2.0.1, along with necessary compatibility patches.
This work depends on Python 3.10 and NumPy:
and all their dependencies; plus a patch to register shapely modules:
In addition it requires libGEOS:
Libs used for build: