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

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

Closed

Conversation

denravonska
Copy link

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

…s are located.

This makes it possible to install the library using pip+git.
@sidcha
Copy link
Member

sidcha commented Feb 13, 2024

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

@denravonska
Copy link
Author

denravonska commented Feb 13, 2024

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 added a commit that referenced this pull request Feb 13, 2024
Python build was reading and parsing CMakeLists.txt from the parent dir
to get the project name and version. Although this reduces one point
that can fall out of sync during a release, it caused python builds to
break when built/installed from pip.

This commit fixes this issue by hard coding the version in python/setup.py
and then extends the release tooling to also update this file.

Related-to: #160
Signed-off-by: Marco Nilsson <[email protected]>
Signed-off-by: Siddharth Chandrasekaran <[email protected]>
@sidcha
Copy link
Member

sidcha commented Feb 13, 2024

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
Copy link
Author

denravonska commented Feb 13, 2024

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
Copy link
Author

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 added a commit that referenced this pull request Feb 13, 2024
Python wheel build happens outside of source tree and hence cannot
access files that are in the git tree. To workaround this issue, let's
vendor all sources needed to build the library into to the package.

Related-to: #160
Signed-off-by: Siddharth Chandrasekaran <[email protected]>
@sidcha
Copy link
Member

sidcha commented Feb 13, 2024

I was doing just that :).

@sidcha
Copy link
Member

sidcha commented Feb 13, 2024

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

@sidcha sidcha closed this Feb 13, 2024
@denravonska
Copy link
Author

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

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.

2 participants