Bug 6474

Update URL information

SPEC: https://github.com/smallorange/ipu6-bin-firmware/blob/main/ipu6-firmware.spec
SRPM: https://github.com/smallorange/ipu6-bin-firmware/releases/download/0.0.1/ipu6-bin-0.0.1-2.fc38.src.rpm

Description:
This rpm spec contains two packages. They are ipu6-bin and ipu6-firmware.

ipu6-bin is used to:
This provides the necessary binaries for Intel IPU6, including IPU6 itself
and iVSC. The library includes necessary image processing algorithms and
3A algorithm for the camera.

ipu6-bin-firmware is used to:
This provides the necessary firmwares for Intel IPU6, including IPU6 and iVSC.

Rpmfusion Account System Username: smallorange

Hi Kate,

Thank you for your work on this.

(In reply to Kate Hsuan from comment #0)
> - Mention that you are seeking a sponsor if you are not a Fedora sponsored
> packager or an RPM Fusion sponsored packager.
> 
> Yes, I'm seeking a reviewer.

The question is about needing a sponsor, since you are already in the Fedora packager group: https://accounts.fedoraproject.org/user/smallorange/  You do *not* need a rpmfusion sponsor. The only thing which is necessary is for your packages to go through the regular review process.

So lets move over to the package review. I have a bunch of initial review remarks:

1. The upstream repo is named ipu6-camera-bins, please also make that the name of the .spec file and the .src.rpm, the main package, the provides, etc. all ipu6-camera-bins.


2. You point to your own fork of ipu6-camera-bins instead of Intel's upstream github repo. For the ipu6-drivers and ivsc-driver (a)kmod packages this makes sense since we need some downstream patches; and this gives the added advantage of being able to tag releases.

But with the ipu6-camera-bins (+ ivsc-firmware) we are just directly consuming whatever Intel drops. So IMHO it would be better to just directly use Intel's github repo here. See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_hosting_services

For how to reference a commit given the lack of releases there. So in the .spec file you would add something like:

%global commit 9874603336d97fd4d12a271485645aaabc7c1be3
%global commitdate 20221021
%global shortcommit %(c=%{commit}; echo ${c:0:7})

%global ivsc_fw_commit 29c5eff4cdaf83e90ef2dcd2035a9cdff6343430
%global ivsc_fw_shortcommit %(c=%{ivsc_fw_commit}; echo ${c:0:7})


Version: 0.0
Release: 1.%{commitdate}git%{shortcommit}%{?dist}

Source0: https://github.com/intel/%{name}/archive/%{commit}/%{name}-%{shortcommit}.tar.gz
Source1: https://github.com/intel/ivsc-firmware/archive/%{ivsc_fw_commit}/ivsc-firmware-%{ivsc_fw_shortcommit}.tar.gz

Typing this outs makes me realize that bundling the ivsc-firmware with the ipu6-firmware is probably not the best of ideas. I think it might be better to just do a separate ivsc-firmware package and have the ivsc-driver-kmod have a "Requires: ivsc-firmware" and have the ipu6-drivers kmod have a "Requires: ipu6-firmware" and then have this package only provide the ipu6-firmware.


3. "%global __brp_check_rpaths %{nil}" is this necessary? Does Intel include rpaths in their pre-compiled .so files? If yes then we should probably just strip the rpaths rather then disabling the rpath checks. You can do this by running "chrpath --delete <file.so>" on the .so files.

4:

"""
Source2:        ld-so-conf-d-ipu6.conf
Source3:        ld-so-conf-d-ipu6ep.conf
"""

The ipu6 and ipu6ep variants of the library provide the same symbols so this is going to cause the variant to get loaded which happened to end up first in the ld.so cache,
which is not what we want.

The plan is to not have these libraries in the ld.so cache / path at all and instead to patch the so build from the ipu6-camera-hal sources with a rpath to the right variant. The libcamerahal.so build from the ipu6-camera-hal sources is the only consumer of this libs, so we can use a rpath inside that .so to pick the right variant of the libs.

We need to do 2 builds of the ipu6-camera-hal (one for both variants) anyways so we will have 2 separate .so-s from each build and we can patch the right rpath into each .so .

TL;DR: please drop these 2 files, we don't want these libraries to be in the standard ld.so path / ld.so cache.

5. You are using 


5. You are currently not creating a -devel subpackage but to build ipu6-camera-hal we are going to need a -devel subpackage. Please add a -devel subpackage which in %install does:

for i in ipu6 ipu6ep; do
  mkdir -p %{buildroot}%{_includedir}/$i
  mkdir -p %{buildroot}%{_libdir}/$i
  cp -pr $i/include/* %{buildroot}%{_includedir}/$i/
  cp -pr $i/lib/pkgconfig %{buildroot}%{_libdir}/$i/
  sed -i \
    -e "s|libdir=/usr/lib|libdir=%{_libdir}|g" \
    -e "s|libdir}|libdir}/$i|g" \
    -e "s|includedir}|includedir}/$i|g" \
    %{buildroot}%{_libdir}/$i/pkgconfig/*.pc
done

And add:

%{_includedir}/ipu6
%{_includedir}/ipu6ep
%{_libdir}/ipu6/pkgconfig/
%{_libdir}/ipu6ep/pkgconfig/

to %files for the -devel subpackage.


6. You may want to simply %install for the libs to just doing:

for i in ipu6 ipu6ep; do
  mkdir -p %{buildroot}%{_libdir}/$i
  cp -pr $i/lib/lib* %{buildroot}%{_libdir}/$i
done

This will also ensure that libgcss.so and libgcss.so.0 stay symlinks which I'm not sure your current approach does. If you merge this with the %install bits needed for the -devel subpackage you get:

for i in ipu6 ipu6ep; do
  mkdir -p %{buildroot}%{_includedir}/$i
  mkdir -p %{buildroot}%{_libdir}/$i
  cp -pr $i/include/* %{buildroot}%{_includedir}/$i/
  cp -pr $i/lib/lib* $i/lib/pkgconfig %{buildroot}%{_libdir}/$i
  sed -i \
    -e "s|libdir=/usr/lib|libdir=%{_libdir}|g" \
    -e "s|libdir}|libdir}/$i|g" \
    -e "s|includedir}|includedir}/$i|g" \
    %{buildroot}%{_libdir}/$i/pkgconfig/*.pc
done

And if we ever get a new variant, then we can just add that to the for. Or if more .so files show up, they will just get installed as well.


Likewise you could simplify the %files for the main ipu6-camera-bins package to:

%files
%license LICENSE
%dir %{_libdir}/ipu6
%{_libdir}/ipu6/*.so*
%{_libdir}/ipu6/*.a
%dir %{_libdir}/ipu6ep
%{_libdir}/ipu6ep/*.so*
%{_libdir}/ipu6ep/*.a


7. I notice that you start the "%package firmware" for the firmware subpackage after theb %files for the main package. This is unusual normally the order in the .spec file is:

<main package tags like Name Version Requires>

%description
<main package decription>


%package <subpackage1>
<subpackage1 tags like Summary, Requires>

%description <subpackage1>
<subpackage decription>


%package <subpackage2>
<subpackage2 tags like Summary, Requires>

%description <subpackage2>
<subpackage decription>


%prep

%build

%install

%files

%files <subpackage1>

%files <subpackage2>

%changelog

That is all I have for now, please prepare a new version addressing these comments.

Regards,

Hans

About:

> Typing this outs makes me realize that bundling the ivsc-firmware with the ipu6-firmware is probably not the best of ideas. I think it might be better to just do a separate ivsc-firmware package and have the ivsc-driver-kmod have a "Requires: ivsc-firmware" and have the ipu6-drivers kmod have a "Requires: ipu6-firmware" and then have this package only provide the ipu6-firmware.

This is talking about having 2 different kmod packages while we agreed to package the ipu6 and ivsc driver upstreams into a singe kmod package since they interdepend on each other during build time.

So assuming that the plan still is to just do a single kmod pkg to solve the interdependency issues, that would mean that that single kmod would get 2 Requires:

Requires: ipu6-camera-bins-firmware
Requires: ivsc-firmware

I still think that given that they have separate upstreams and that we thus need to reference 2 different commit hashes to make clear which version we are packaging that putting the ivsc-firmware in its own (small) package is better then adding it to this package.

Thank you for the comments :)

I'll revise the source URL to Intel's repo.

You are right.
We can package one kmod rpm for both ivsc and ipu6 and two separate packages for ivsc-firmware and ipu6-camera-bin.

For iVSC firmware, I'll create a new rpm spec file for it.

Updated version:
SPEC: https://github.com/smallorange/ipu6-bin-firmware/blob/main/ipu6-camera-bins.spec
SRPM: https://github.com/smallorange/ipu6-bin-firmware/releases/download/0.0.1/ipu6-camera-bins-0.0-1.20221112git4694ba7.fc38.src.rpm

Modifications:
This package contains three packages, including ipu6-camera-bins, ipu-camera-bins-devel, and ipu6-camera-bins-firmware.

ipu6-camera-bins is the binary library file.

ipu6-camera-bins-devel contains the necessary header files for development.

ipu6-camera-bins-firmware includes IPU6 firmwae.

Also, ipu6-camera-bins provides a virtual Provides- intel-ipu6-kmod-common to fulfill the akmod package https://bugzilla.rpmfusion.org/show_bug.cgi?id=6469

(In reply to Kate Hsuan from comment #6)
> Updated version:
> SPEC:
> https://github.com/smallorange/ipu6-bin-firmware/blob/main/ipu6-camera-bins.
> spec
> SRPM:
> https://github.com/smallorange/ipu6-bin-firmware/releases/download/0.0.1/
> ipu6-camera-bins-0.0-1.20221112git4694ba7.fc38.src.rpm

Thanks for the new version, this looks pretty good to me, just a few small items remaining to fix:

1. You use:

%setup -q -c
cp %{name}-%{commit}/LICENSE ./
cd %{name}-%{commit}
...

You can replace this with just:

%setup -q -n %{name}-%{commit}
...

And then you can also drop all the %{name}-%{commit}/ prefixes of source files in %install.

2. License: tag should be Proprietary

3. Please use the same level of indentation after "Summary: " as you do after the other tags.

4. Please drop the "Provides: %{name}-<subpkg> = %{version}-%{release}" tags from the subpackages, rpm will add these automatically

5. Please drop the "Requires:         %{name} = %{version}-%{release}" from the firmware subpackage, it does not actually require anything from the main package. 

6. Please move the "Requires:         %{name} = %{version}-%{release}" for the
devel subpackage to above the "%description devel".

7. Please drop the "%license LICENSE" from "%files devel", since the devel subpackage Requires the main package there is no need for it to have a separate copy of the LICENSE.


Other then these small tweaks I believe that you should drop the intel-ipu6-kmod-common virtual Provides and that we will need to revive the intel-ipu6-kmod-common package review and have that be a virtual package (empty %files list) which can then have Requires on all the bits which we will eventually need, so from the top of my head:

ipu6-camera-bins-firmware
ivsc-firmware
ipu6-camera-bins
ipu6-camera-hal
gstreamer10-icamerasrc
v4l2-loopback-kmod
v4l2-relayd

Given this long and potentially changing list of Requires necessary to get a fully working stack I believe just maintaining this inside an otherwise empty intel-ipu6-kmod-common is probably best, sorry for changing my mind on this.

Note AFAIK the kmod generator will automatically add a dep on intel-ipu6-kmod-common and we cannot inject other Requires, which is why we need this virtual package to list the actual Requires.

p.s.

One more remark, when you do a new version during review you should bump the Release:, so the next one becomes 2.%{commitdate}git%{shortcommit}%{?dist} and add an entry to %changelog describing the changes in this case that can be something like:

* xxx xx xxx 2022 Kate Hsuan <hpa@redhat.com> - 0.0-2.20221112git4694ba7
- Small tweaks as a result of pkg-review (rf#6474)

I also just noticed your changelog format is wrong, you write:

* Tue Oct 25 2022 Kate Hsuan <hpa@redhat.com> - 0.0.1

But that should be:

* Tue Oct 25 2022 Kate Hsuan <hpa@redhat.com> - 0.0-1.20221112git4694ba7

rpmlint should have warned you about this, please run:

rpmlint <path>/*.src.rpm <path>/*.x86_64.rpm 

on the next version before submitting it before review and:

1. Fix any warnings which you can easily fix

2. Include a copy of the rpmlint output in a comment here for the rest

Note rpmlint will likely complain quite a bit, esp. with a package without source like this, feel free to ignore rpmlint output when it does not seem to make sense (or when you don't know what it is trying to tell you).

Thanks for the comments.

The updated version can be found as follows:

SPEC: https://github.com/smallorange/ipu6-bin-firmware/blob/main/ipu6-camera-bins.spec
SRPM: https://github.com/smallorange/ipu6-bin-firmware/releases/download/0.0.1/ipu6-camera-bins-0.0-2.20221112git4694ba7.fc38.src.rpm

rpmlint output is shown in attachment.

Created attachment 2444 [details]
rpmlint output

The rpmlint result for revision #2.

Hi Kate,

Below is a full review of this package, see the Must Fix section at the end for a summary of things to fix.

Regards,

Hans


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains static libraries but this is simply the way the binary-only
     upstream source works, so this is fine in this case
[x]: The main (non -devel) pkg contains unversioned .so files, these are in
     a private %_libdir subdirectory and again this is just the way the provided
     binaries are.
[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 NOT licensed with an open-source compatible license
     (it contains binaries which are only distributed in binary form)
     this is why this is intended for rpmfusion-nonfree.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[-]: %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 (for rpmfusion)
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: 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.
[-]: Package contains systemd file(s) if in need.
[-]: Useful -debuginfo package or justification otherwise.
[x]: Package uses ExclusiveArch, since the upstream binaries are only available for that arch
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[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.
[-]: 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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.


Rpmlint
-------
There are a ton of rpmlint warnings/errors for this package, but this
all have to do with the upstream provided binaries being what they
are, so these can all be waived / ignored.


===== Must Fix =====

[!] The .a files are static libraries which are only used link-time never run-time.
    These must be nmoved to the -devel package
[!] You have 2 'for i in ipu6 ipu6ep; do' loops and even do:
    'mkdir -p %{buildroot}%{_libdir}/$i' twice. Please fold this into 1 loop and
    drop the duplicate mkdir.
[!] There are issues with directory ownership. Add:
    %dir %{_libdir}/ipu6
    %dir %{_libdir}/ipu6ep
    To the main pkg %files; and 
    %dir /lib/firmware
    %dir /lib/firmware/intel
    To the -firmware subpkg %files.
[!] The -devel subpackage must have a  "Requires: %{name}%{?_isa} = %{version}-%{release}"
    added to it so that installing it brings in the main package and so that
    the -devel and main package are version locked
[!] De install command for the firmware files should be passed "-p" to preserve their
    timestamps

Hi Hans,

SPEC: https://raw.githubusercontent.com/smallorange/ipu6-bin-firmware/main/ipu6-camera-bins.spec
SRPM: https://github.com/smallorange/ipu6-bin-firmware/releases/download/0.0-4/ipu6-camera-bins-0.0-4.20221112git4694ba7.fc38.src.rpm

The "MUST FIX" items were fixed.

Moreover, for the -firmware package, the firmware path was changed to "/usr/lib/firmware".
%dir was added to each package.

Please review it again.

Thank you

BR,
Kate

Thanks this looks good to me now: approved.

Please follow the next steps at:

https://rpmfusion.org/Contributors#Your_package_gets_approved


2 small remaining nitpicks, please fix these after importing the package:

1. Please use the same indentation between a "Tag-name:  " and the value for that tag-name everywhere and please keeps Tags together rather then adding unnecessary empty lines inside a block of tags, specifically please change:

%package devel
Summary:        IPU6 header files for development.

Requires: %{name}%{?_isa} = %{version}-%{release}

to:

%package devel
Summary:        IPU6 header files for development.
Requires:       %{name}%{?_isa} = %{version}-%{release}


2. Since the main package already owns %{_libdir}/ipu6 and %{_libdir}/ipu6ep; and the -devel subpackage requires the main package there is no need for the -devel subpackage to own these too. Please drop these 2 lines from "%files devel":

%dir %{_libdir}/ipu6
%dir %{_libdir}/ipu6ep

Thank you. I start the next tasks for the new package.
I'm not in the packager group so I have to wait for the approval.

Update spec files and SRPM

SPEC: https://github.com/smallorange/ipu6-bin-firmware/raw/main/ipu6-camera-bins.spec
SRPM: https://github.com/smallorange/ipu6-bin-firmware/releases/download/0.0-5/ipu6-camera-bins-0.0-5.20221112git4694ba7.fc38.src.rpm

Package request has been processed.
Please make sure you use the correct spec name and summary for the bugzilla request title.
Also use the correct namespace for pkgdb requests.

(In reply to leigh scott from comment #15)
> Package request has been processed.
> Please make sure you use the correct spec name and summary for the bugzilla
> request title.
> Also use the correct namespace for pkgdb requests.

Thank you!
Name, BZ title, and namespace are good. :)