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

edgeai-apps-utils: add support to build edgeai-apps-utils package #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsuhaas22
Copy link
Collaborator

No description provided.

dh $@

override_dh_auto_configure:
mkdir build/ && cd build/ && cmake ..
Copy link
Collaborator

Choose a reason for hiding this comment

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

Current debhelper should be able to build cmake projects automatically, these overrides shouldn't be needed.

@@ -0,0 +1 @@
version=3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty watch files don't help, just remove this file.


mkdir -p /opt

git clone https://git.ti.com/git/edgeai/edgeai-apps-utils /opt/edgeai-apps-utils
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nak, just copy the source to somewhere more standard, like /usr/src or /usr/share. Have it as part of the package content, no .postinst downloading/magic please, packages should install the same without network access.

Upstream-Contact: Texas Instruments
Source: https://git.ti.com/cgit/edgeai/edgeai-apps-utils

Files: include/edgeai_dl_color_convert_armv8_utils.h
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this file is auto-generated, but you should fix it up a little. All files have the same license, use wildcard * to collect them all. Then the License is not "UNKNOWN" it is a TI BSD-3-clause. See how I did the same fix for k3conf:

c9baa29

Homepage: https://git.ti.com/cgit/edgeai/edgeai-apps-utils
Rules-Requires-Root: no

Package: libedgeai-apps-utils0.1.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't have versions in the package name unless we expect to have multiple version installed at the same time.

@@ -0,0 +1,5 @@
edgeai-apps-utils (0.1+git20240205+8e84be376-1) UNRELEASED; urgency=low
Copy link
Collaborator

Choose a reason for hiding this comment

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

UNRELEASED causes issues with some tools, every time these files are updated in the main branch they would be built and "released", so you can just set this to unstable or bookworm.

@jsuhaas22
Copy link
Collaborator Author

Thanks @glneo for the review. I have done the changes. Not sure if I got the copyright file right.

@glneo
Copy link
Collaborator

glneo commented Feb 15, 2024

Looks much better, thanks!

@jsuhaas22
Copy link
Collaborator Author

Removed the source code from the package.

@@ -0,0 +1,6 @@
#!/usr/bin/make -f

export SOC=am62x
Copy link
Collaborator

Choose a reason for hiding this comment

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

So here is a problem, for some reason they made part of this program SoC specific, instead of just doing runtime detection of the platform. This means they need a different binary compiled for each and every SoC. And you would then need a different package for each supported SoC. That is bad software design. They might get away with it in Yocto where the whole system is rebuild for a given machine, but not normal distros.

Let's fix this at the source, go let the edgeai-apps-utils folks know this won't work. They need fix their compile-time hard-coding and instead do runtime detection, just like we do in k3conf. They can use that as an example even.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense.
I talked to the edgeai team. They have agreed to do it, but they said that it will require some work. They said that it may not be possible in the near-future, but they'll try to squeeze it in 9.2 release.

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