Bug 1854729
Summary: | Review Request: nispor - API for network state query written in rust | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Gris Ge <fge> |
Component: | Package Review | Assignee: | Robert-André Mauchin 🐧 <zebob.m> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | ferferna, package-review, till, zebob.m |
Target Milestone: | --- | Flags: | zebob.m:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | nispor-0.5.0-2.fc34 | Doc Type: | If docs needed, set a value |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2020-09-07 07:52:19 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 1860781, 1860784, 1860785, 1860787, 1860790, 1860792, 1860794, 1860796, 1860797, 1860798, 1860800, 1860801, 1860802, 1860803, 1860805, 1860806 | ||
Bug Blocks: |
Description
Gris Ge
2020-07-08 06:31:40 UTC
The rust crates src rpm is created by this script: ```bash #!/bin/bash -ex mkdir rust-$1 || true cd rust-$1 rust2rpm -s $1 cp *.crate ~/rpmbuild/SOURCES/ sed -i -e's/bcond_without/bcond_with/' rust-$1.spec sed -i -e'/%doc ..\/README.md/d' rust-$1.spec rpmbuild -bs rust-$1.spec scp ~/rpmbuild/SRPMS/rust-$1*.src.rpm gris-laptop:/tmp ``` Notes: * Disabled the `check` as it will introduce a long list of dependent crates. * Removed the `%doc ..README.md` as that is does not exists. -No license, no docs? %files %doc AUTHORS CHANGELOG DEVEL.md README.md %license LICENSE %{_bindir}/npc %{_bindir}/npd %{_unitdir}/nispor.socket %{_unitdir}/nispor.service %files -n python3-%{name} %license LICENSE %{python3_sitelib}/nispor.so %files -n rust-%{name}-devel %license LICENSE %{cargo_registry}/%{name}-%{version_no_tilde}/ %files -n rust-%{name}+default-devel %ghost %{cargo_registry}/%{name}-%{version_no_tilde}/Cargo.toml I can't build due to missing deps: DEBUG util.py:621: Problem 1: nothing provides requested (crate(netlink-packet-route/default) >= 0.3.0 with crate(netlink-packet-route/default) < 0.4.0) DEBUG util.py:621: Problem 2: nothing provides requested (crate(netlink-sys/default) >= 0.3.0 with crate(netlink-sys/default) < 0.4.0) DEBUG util.py:621: Problem 3: nothing provides requested (crate(pyo3/extension-module) >= 0.11.1 with crate(pyo3/extension-module) < 0.12.0) DEBUG util.py:621: Problem 4: nothing provides requested (crate(pyo3/macros) >= 0.11.1 with crate(pyo3/macros) < 0.12.0) DEBUG util.py:621: Problem 5: nothing provides requested (crate(rtnetlink/default) >= 0.3.0 with crate(rtnetlink/default) < 0.4.0) The dependent rust crates are stored in: https://copr.fedorainfracloud.org/coprs/cathay4t/nispor/builds/ I will add the license and etc. (In reply to Gris Ge from comment #4) > The dependent rust crates are stored in: > https://copr.fedorainfracloud.org/coprs/cathay4t/nispor/builds/ > Shouldn't you open new Review Requests for these crates then? Otherwise it can't be built in Rawhide. It would be nice to have egg-info for the python module. (In reply to Till Maas from comment #6) > It would be nice to have egg-info for the python module. The setuptool-rust can generate that file, but that would introduce another dependency which is no ideal for CentOS/RHEL. I will try to write a simpler version of `setuptool-rust` for this, but not a priority. New SPEC file uploaded: https://fedorapeople.org/~cathay4t/nispor/nispor.spec New SRPM: https://download.copr.fedorainfracloud.org/results/cathay4t/nispor/fedora-rawhide-x86_64/01538834-nispor/nispor-0.1.1-2.fc33.src.rpm Changes: * Included license and documents. Hi Fernando, Can you take review on this also? Thank you. The SPEC file looks good to me. I am not sure if it is a good idea to have the dependent crates in a Copr repository. Will this force all the users to enable the copr repository before installing nispor? (In reply to Fernando F. Mancera from comment #10) > The SPEC file looks good to me. I am not sure if it is a good idea to have > the dependent crates in a Copr repository. Will this force all the users to > enable the copr repository before installing nispor? The copr repo is just demonstration it works. Packaging rust crates is rule of Fedora rawhide. And for Fedora stable rpm, it will just use the compiled package built from the Fedora rawhide buildroot. The rust package is static linking, once compiled, it does not has runtime dependency besides glibc. (In reply to Gris Ge from comment #11) > (In reply to Fernando F. Mancera from comment #10) > > The SPEC file looks good to me. I am not sure if it is a good idea to have > > the dependent crates in a Copr repository. Will this force all the users to > > enable the copr repository before installing nispor? > > The copr repo is just demonstration it works. > > Packaging rust crates is rule of Fedora rawhide. > And for Fedora stable rpm, it will just use the compiled package built from > the Fedora rawhide buildroot. > > The rust package is static linking, once compiled, it does not has runtime > dependency besides glibc. Oh, in that case, it looks good to me. Thanks for explaining. Will create new package request for dependent rust crates. Dependent rust crates package created at: https://bugzilla.redhat.com/buglist.cgi?bug_id=1854729&bug_id_type=anddependson&list_id=11245189&query_format=advanced They could be grouped into two: * rust-rtnetlink and its dependencies * rust-pyo3 and its dependencies New rpm SPEC uploaded: https://fedorapeople.org/~cathay4t/nispor/nispor.spec New srpm: https://download.copr.fedorainfracloud.org/results/cathay4t/nispor/fedora-rawhide-x86_64/01612205-nispor/nispor-0.3.0-1.fc34.src.rpm Changes: * Upgrade to 0.3.0 * `python3-nmstate` now have egginfo files. * Removed the need of rust-pyo3. So currently, missing dependent rust-crates is rust-netlink and its subpackages. Please also review these dependent rust-crates: * Bug 1860781 Review Request: rust-netlink-packet-core * Bug 1860784 Review Request: rust-netlink-packet-route * Bug 1860785 Review Request: rust-netlink-packet-utils * Bug 1860787 Review Request: rust-netlink-proto * Bug 1860790 Review Request: rust-netlink-sys * Bug 1860792 Review Request: rust-rtnetlink Since I am actively working with rust-netlink upstream, I will be the rpm maintainer of above packages also. - Add comments explaining why patches are needed Patch1: 0001-varlink-Upgrade-to-rust-varlink-11.patch Patch2: 0002-Makefile-Better-handling-on-libdir.patch - Please justify why you disabled the tests: %bcond_with check - env SKIP_PYTHON_INSTALL=1 DESTDIR=%{buildroot} PREFIX=/usr \ LIBDIR=%{_libdir} make install → env SKIP_PYTHON_INSTALL=1 PREFIX=%{_prefix} \ LIBDIR=%{_libdir} %make_install - Add license in these packages as well %files -n python3-%{name} %license LICENSE %{python3_sitelib}/nispor* %files -n rust-%{name}-devel %license LICENSE %{cargo_registry}/%{name}-%{version_no_tilde}/ - In order to avoid unintentional soname bump, we recommend not globbing the major soname version: %{_libdir}/libnispor.so.0* - Add arch specific info for your Requires %package devel Summary: %{summary} Requires: nispor%{?_isa} = %{?epoch:%{epoch}:}%{version}-%{release} -Error while installing DEBUG util.py:621: Problem: conflicting requests DEBUG util.py:621: - nothing provides libnispor.so()(64bit) needed by nispor-devel-0.3.0-1.fc34.x86_64 Not sure what's causing this: rpm -q --provides -p nispor-devel-0.3.0-1.fc34.x86_64.rpm | sort -f | uniq -c 21:19:36 1 nispor-devel = 0.3.0-1.fc34 1 nispor-devel(x86-64) = 0.3.0-1.fc34 rpm -q --requires -p nispor-devel-0.3.0-1.fc34.x86_64.rpm | sort -f | uniq -c 21:20:01 1 libnispor.so()(64bit) 1 nispor(x86-64) = 0.3.0-1.fc34 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsZstd) <= 5.4.18-1 It seems to me the library is badly compiled as it doesn't have a soname: objdump -p libnispor.so.0.3.0 | grep SONAME Could be why it isn't requiring libnispor.so()(64bit) instead of libnispor.so.0.3.0()(64bit) - libnispor.so.0.3.0 is not marked as 0755: -rw-r--r--. 1 bob bob 14M Aug 25 21:16 libnispor.so.0.3.0 Use: chmod 0755 %{buildroot}%{_libdir}/libnispor.so.0.3.0 in install, but or use the patch I sent upstream to fix the Makefile, line 87: install -v -D -m644 $(CLIB_SO_DEV_RELEASE) \ $(DESTDIR)$(LIBDIR)/$(CLIB_SO_FULL) Should be 0755: https://github.com/nispor/nispor/pull/13 On a side note this allow you to reenable the debuginfo, now that the library is executable, debuginfo will be correctly generated. - Also you should fix the Makefile to use "install -p" for files to keep the timestamps. Hi Robert-André Mauchin, Thanks for the review. Still fixing `libnispor.so()(64bit)` issue, the `SONAME` does not helps. New rpm SPEC uploaded: https://fedorapeople.org/~cathay4t/nispor/nispor.spec New srpm: https://download.copr.fedorainfracloud.org/results/cathay4t/nispor/fedora-rawhide-x86_64/01635398-nispor/nispor-0.4.0-1.fc34.src.rpm Changes: * Upgrade to 0.4.0 * Add license file to `nispor-devel`, `python3-nispor` and `rust-nispor-devel`. * Fixed the SONAME which impact on the `nispor-devel` requirements. The root cause is %cargo_prep override upstream `.cargo/config.toml`. Workaround by merged upstream rustflags into %cargo_prep. Thank you! nispor.src:124: W: libdir-macro-in-noarch-package (main package) %{_libdir}/libnispor.so.* nispor.src:134: W: libdir-macro-in-noarch-package devel %{_libdir}/libnispor.so These packages should be noarch since you shipping arched binary code in it. python3-nispor.noarch: E: useless-provides python3dist(nispor) rust-nispor-devel.noarch: E: useless-provides crate(nispor) rust-nispor+default-devel.noarch: E: useless-provides crate(nispor/default) Not exactly sure what is causing that error to show up, but in all cases, Provides: python3dist(%{name}) = %{version} Provides: crate(%{name}) = %{version} Provides: crate(%{name}/default) = %{version} this shouldn't be necessary, they should be autodetected. Thanks for the review. I have updated the rpm SPEC: https://fedorapeople.org/~cathay4t/nispor/nispor.spec Source rpm: https://fedorapeople.org/~cathay4t/nispor/nispor-0.4.0-2.fc34.src.rpm Copr repo for testing: https://copr.fedorainfracloud.org/coprs/cathay4t/nispor/ Package approved. (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/nispor Hi Robert-André Mauchin, Do you know how to include package into Fedora 33? The rawhide build is named as f34. Never mind. I missed the deadline of Fedora 33. |