goToMain / libosdp

Implementation of IEC 60839-11-5 OSDP (Open Supervised Device Protocol); provides a C library with support for C++, Rust and Python3
https://libosdp.sidcha.dev
Apache License 2.0
137 stars 71 forks source link

Use the path of setup.py instead of cwd to determine where the sources are located #160

Closed denravonska closed 7 months ago

denravonska commented 7 months ago

This makes it possible to install the library using pip+git in setup.py/requirements.txt.

sidcha commented 7 months ago

Thanks for the PR. The CI failure looks legit and a lot similar to the issue you've created (#159).

denravonska commented 7 months ago

The CI failure is an interesting one. The build module does a temp copy of the Python sources to /tmp, then executes the build. This worked before when the C sources were resolved from cwd.

I'm not sure what the correct solution is here. One workaound could be to expose a BUILD_PATH environment variable that's set in the CI and have setup.py pick it up, using the path of setup.py as fallback.

sidcha commented 7 months ago

I think, I fixed one other problem than that was in this PR in 42399bb. That also implicitly merged this change (I added your Signoff-by:).

denravonska commented 7 months ago

That should work. My proposal would be something like

-current_dir = os.path.dirname(os.path.realpath(__file__))
-build_dir = os.path.abspath(os.path.join(current_dir, "build"))
-root_dir = os.path.abspath(os.path.join(current_dir, ".."))
+from setuptools import Extension, setup
+
+# Allow the caller to specify the location of the source tree while using the
+# location of setup.py as fallback. This allows the CI to build wheel packages
+# using the "build" package while simultaneously opening up for pip
+# installations over git+https.
+setup_py_dir = os.path.dirname(os.path.realpath(__file__))
+source_dir = os.environ.get("SOURCE_DIR", setup_py_dir)
+build_dir = os.path.abspath(os.path.join(source_dir, "build"))
+root_dir = os.path.abspath(os.path.join(source_dir, ".."))

Where the location of setup.py could be specified by CI (fixing the wheel building) while still allowing for pip install "git+https://..." as a dependency specification.

However, if you're happy with the hard coded version number I'm happy :)

Edit: Oh, you change the version at build time. Gotcha!

denravonska commented 7 months ago

There are still references to the C sources. I'm not sure this can be resolved without pointing setup.py to the sources, or by making the relevant C sources a part of the Python distribution. The latter has the benefit of also working when pulling from Pypi.

sidcha commented 7 months ago

I was doing just that :).

sidcha commented 7 months ago

With that, I think we can close this PR. Please feel free to reopen if you have any further issues.

denravonska commented 7 months ago

That seems to have worked out great. Thanks!

(env) marco@dev:~/temp$ pip install "git+https://github.com/goToMain/libosdp.git#egg=libosdp&subdirectory=python"
...
Successfully built libosdp
Installing collected packages: libosdp
Successfully installed libosdp-2.4.0
(env) marco@dev:~/temp$ ls env/lib
lib/   lib64/ 
(env) marco@dev:~/temp$ ls env/lib/python3.11/site-packages/
_distutils_hack  distutils-precedence.pth  libosdp-2.4.0.dist-info  osdp  osdp_sys.cpython-311-x86_64-linux-gnu.so  pip  pip-23.0.1.dist-info  pkg_resources  setuptools  setuptools-66.1.1.dist-info