[Bug 6534] Review request: icamerasrc - GStreamer plugin for Intel IPU6
https://bugzilla.rpmfusion.org/show_bug.cgi?id=6534
Hans de Goede changed:
What|Removed |Added
CC||h...@hansg.org
--- Comment #2 from Hans de Goede ---
I have started working on reviewing this. For starters I have a list of various
things which need to be changed which stood out while just trying to build this
/ looking at the spec file.
Please prepare a new version addressing these, then I will run through the
detailed review checklist after that.
1. Please rename the package (and *.spec file and .src.rpm file) to
gstreamer1-plugins-icamerasrc), following the standard pkg naming scheme for
gstreamer plugins.
Note please also update the Summary of this bug to match.
2. %build must not rely on the ./configure invocation done by autogen.sh,
looking at autogen.sh, you can replace the autogen.sh invocation with
"autoreconf --force --install --make", so that it does not run configure at
all. And please move this to the %prep section
3. And then for %build use:
%build
export CHROME_SLIM_CAMHAL=ON
export STRIP_VIRTUAL_CHANNEL_CAMHAL=ON
%configure
%make_build
Note I also dropped the "export PKG_CONFIG_PATH="/usr/lib64/pkgconfig" as that
is not necessary.
4. The Makefile comes with proper "make install" support, so %install can be
simplified to just:
%install
%make_install
Note this will change the install location of some of the -devel subpackage
files, adjust %files accordingly.
5. Since we use a libcamhal.so symlink in ipu6-camera-hal to pick the right
libcamhal.so for the hw, there is no need to play any rpath tricks, so all the
rpath stuff can be dropped from %install (The entire %install should only have
the single "%make_install" command remaining).
And these 2 lines at the top should be dropped too:
#ignore rpath check for a invalid path
%global __brp_check_rpaths %{nil}
I might have some more remarks after this, but lets start with these
improvements which should greatly improve the .spec file.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.___
rpmfusion-developers mailing list -- rpmfusion-developers@lists.rpmfusion.org
To unsubscribe send an email to rpmfusion-developers-le...@lists.rpmfusion.org
[Bug 6508] Review request: ipu6-camera-hal - Hardware abstraction layer for Intel IPU6
https://bugzilla.rpmfusion.org/show_bug.cgi?id=6508 Hans de Goede changed: What|Removed |Added Attachment #2468|0 |1 is obsolete|| --- Comment #21 from Hans de Goede --- Created attachment 2474 --> https://bugzilla.rpmfusion.org/attachment.cgi?id=2474=edit 60-intel-ipu6.rules -- You are receiving this mail because: You are on the CC list for the bug.___ rpmfusion-developers mailing list -- rpmfusion-developers@lists.rpmfusion.org To unsubscribe send an email to rpmfusion-developers-le...@lists.rpmfusion.org
[Bug 6508] Review request: ipu6-camera-hal - Hardware abstraction layer for Intel IPU6
https://bugzilla.rpmfusion.org/show_bug.cgi?id=6508 --- Comment #20 from Hans de Goede --- I do have one request / improvement (which can be done after importing the package). Please add: KERNEL=="ipu-psys0", TAG+="uaccess" To 60-intel-ipu6.rules, this avoids the need to chmod /dev/ipu-psys0 before running gst-launch as normal user to test the camera. I'll also attach the updated 60-intel-ipu6.rules file. -- You are receiving this mail because: You are on the CC list for the bug.___ rpmfusion-developers mailing list -- rpmfusion-developers@lists.rpmfusion.org To unsubscribe send an email to rpmfusion-developers-le...@lists.rpmfusion.org
[Bug 6508] Review request: ipu6-camera-hal - Hardware abstraction layer for Intel IPU6
https://bugzilla.rpmfusion.org/show_bug.cgi?id=6508
Hans de Goede changed:
What|Removed |Added
Flags|fedora-review? |fedora-review+
--- Comment #19 from Hans de Goede ---
Full review done:
Package Review
==
Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
= MUST items =
C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
Generic:
[x]: Package is licensed with an open-source compatible license and meets
other legal requirements as defined in the legal section of Packaging
Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[-]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
one supported primary architecture.
[x]: Rpmlint is run on all rpms the build produces.
Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package is included in %license.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
%{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
(~1MB) or number of files.
Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local
= SHOULD items =
Generic:
[-]: If the source package does not include license text(s) as a separate
file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
justified.
[-]: Sources are verified with gpgverify first in %prep if upstream
publishes signatures.
Note: gpgverify is not used.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
$RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq,
[Bug 6469] Review request: intel-ipu6-kmod - The driver for Intel IPU6 MIPI camera
https://bugzilla.rpmfusion.org/show_bug.cgi?id=6469
--- Comment #11 from Hans de Goede ---
Nicolas,
Thank you for your help with reviewing this.
Yes, I will take care of the userspace packages. It might be helpful for me to
explain what the entire stack looks like:
1. A set of out of tree open-source drivers (this review request)
2. ipu6-camera-bins-firmware, firmware for 1. (review bug 6474)
3. ivsc-firmware, more firmware for 1. (review bug 6491)
4. ipu6-camera-bins set of binary only x86_64 libraries talking to 1. (review
bug 6474)
5. ipu6-camera-hal opensource (apache-2.0) library layer over ipu6-camera-bins
(review bug 6508)
6. gstreamer1-plugins-icamerasrc gstreamer plugin building on top of 5. (review
bug 6534)
This is the minimum stack after this the camera can be tested with gst-launch.
But to actually get it to work we need another program on top called
v4l2-relayd which uses the v4l2-loopback kernel module. This will listen for
userspace opening the loopback /dev/video0 device and then it will start a
gstreamer pipeline for this
(review bug 6529).
We want users to be able to just do a "sudo dnf install akmod-intel-ipu6" and
then get a fully working stack.
Nicolas, so I guess this means that we need to add the following Requires to
ipu6-camera-bins since that provides intel-ipu6-kmod-common ? :
Requires: ipu6-camera-bins-firmware
Requires: ivsc-firmware
Requires: gstreamer1-plugins-icamerasrc
Requires: v4l2-relayd
(note gstreamer1-plugins-icamerasrc will bring in ipu6-camera-hal).
Having these there feels a bit weird which is why I suggested a stub
intel-ipu6-kmod-common package to Kate, but if the usual way for kmods is to
just put these inside ipu6-camera-bins then that is fine with me too.
> So this package should have:
> Requires:intel-ipu6-kmod >= %{?epoch}:%{version}
I agree on the "Requires: intel-ipu6-kmod" but I think it should be unversioned
there is no tight version coupling here and there actually is nothing
resembling proper versioning from upstream at all. So I think we want just:
Requires: intel-ipu6-kmod
added on top of the Requires I mentioned above.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.___
rpmfusion-developers mailing list -- rpmfusion-developers@lists.rpmfusion.org
To unsubscribe send an email to rpmfusion-developers-le...@lists.rpmfusion.org
