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

Added DepthAI package and required fixes #1059

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

themarpe
Copy link
Contributor

@themarpe themarpe commented Dec 20, 2023

This contains a couple of parts to get everything in place:

This is an iteration/simplification of: #1010, to not require bump of API. To circumvent that, package had to be patch in the background to accomodate for older NDA and explicit link to -lpython3.x had to be added as a patch.

Otherwise a more straightforward PR and still functions as expected

@mhsmith
Copy link
Member

mhsmith commented Dec 20, 2023

Thanks, and I'm sorry for not responding to your previous PR – it touched a number of complex areas, so it wasn't easy for me to find time to review. This one looks a lot easier to handle.

One of those areas which has become a bit simpler in the last few days is actually installing and running CMake. Please merge from the master branch to pick up these:

  • I've removed the special case for cmake in the build requirements section of meta.yaml, and made CMake just another package which is installed from PyPI. This means that recipes which list CMake in their meta.yaml file are now required to give a CMake version number. However, it turns out that many Python packages that use CMake already list it in their pyproject.toml file, in which case the meta.yaml entry should just be removed.

  • build-wheel.py now sets the CMAKE_TOOLCHAIN_FILE environment variable which is recognized by CMake 3.21 and later. So that can probably be removed from the patch.

Also, to add a new package, we need a test script to verify that it works. Please do the following:

  • Follow the instructions in "Testing a package" in the README.
  • Push your test script to this PR.
  • Post a comment saying which Python versions and ABIs you have tested.

Comment on lines 3 to 9
version: "2.24.0.0" # If a release is picked
# version: "2.24.0.0.dev0+7b57b28305368582d004d5c6ec2cffb66562f2e0"

source:
git_url: https://github.com/luxonis/depthai-python.git
git_rev: 7b57b28305368582d004d5c6ec2cffb66562f2e0
# path: [path/to/depthai-python]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what "If a release is picked" means.

Also, if the commented-out version and path lines serve some purpose, please add more comments to explain what that is, or otherwise remove them.

@mhsmith
Copy link
Member

mhsmith commented Dec 20, 2023

To circumvent that, package had to be patch in the background to accomodate for older NDA

What does "NDA" mean?

@themarpe
Copy link
Contributor Author

themarpe commented Dec 28, 2023

Thanks for the comments! Addressed & force pushed!

To circumvent that, package had to be patch in the background to accomodate for older NDA

What does "NDA" mean?

Sorry, meant NDK here.


Added test case, tested against

  • python3.8 arm64-v8a

@mhsmith mhsmith merged commit db1f9bf into chaquo:master Jan 16, 2024
@mhsmith
Copy link
Member

mhsmith commented Jan 16, 2024

It looks like since you explicitly added libpython to the link in the CMakeLists file, the changes to the toolchain file were no longer necessary, so I've removed them.

The cmake-example recipe had to be fixed in the same way – removing the --no-undefined flag allowed it to build, but without the libpython link, the module still couldn't be loaded at runtime.

I was unable to add these changes to this PR because you apparently disabled the "Allow edits from maintainers" option when you created it. Please leave this enabled in future PRs.

This package is now in the public repository for Python 3.8 - 3.12. Thanks for the contribution!

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

Successfully merging this pull request may close these issues.

cmake-example build fails
2 participants