Bug 1958330 - Review Request: rust-libbpf-sys - Rust bindings to libbpf from the Linux kernel
Summary: Review Request: rust-libbpf-sys - Rust bindings to libbpf from the Linux kernel
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Robert-André Mauchin 🐧
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1958352
TreeView+ depends on / blocked
 
Reported: 2021-05-07 16:18 UTC by Davide Cavalca
Modified: 2021-08-04 22:46 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-08-04 22:46:52 UTC
Type: ---
Embargoed:
zebob.m: fedora-review+


Attachments (Terms of Use)

Description Davide Cavalca 2021-05-07 16:18:16 UTC
Spec URL: https://dcavalca.fedorapeople.org/review/rust-libbpf-sys/rust-libbpf-sys.spec
SRPM URL: https://dcavalca.fedorapeople.org/review/rust-libbpf-sys/rust-libbpf-sys-0.3.0~1-1.fc35.src.rpm

Description:
Rust bindings to libbpf from the Linux kernel.

Fedora Account System Username: dcavalca

Comment 1 Davide Cavalca 2021-05-07 16:18:18 UTC
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=67433664

Comment 2 Robert-André Mauchin 🐧 2021-05-09 16:36:28 UTC
 - You must install the LICENSE with %license in %files

%package        devel
Summary:        %{summary}
BuildArch:      noarch

%description    devel %{_description}

This package contains library source intended for building other packages
which use "%{crate}" crate.

%files          devel
%license LICENSE
%doc README.md
%{cargo_registry}/%{crate}-%{version_no_tilde}/


 - Can't you unbundle libbpf? We do have a libbpf-static in Fedora.

 - These should be required in the devel subpackages too:

%package        devel
Summary:        %{summary}
BuildArch:      noarch
Requires:       elfutils-libelf-devel
Requires:       zlib-devel

so that when a package needs rust-libbpf-sys to build, it can fetch its lib dependencies.

Comment 3 Davide Cavalca 2021-05-09 17:48:11 UTC
Spec URL: https://dcavalca.fedorapeople.org/review/rust-libbpf-sys/rust-libbpf-sys.spec
SRPM URL: https://dcavalca.fedorapeople.org/review/rust-libbpf-sys/rust-libbpf-sys-0.3.0~1-2.fc35.src.rpm

Changelog:
- Install license
- Unbundle libbpf and use the system one instead
- Add missing Requires to devel subpackages

Comment 4 Robert-André Mauchin 🐧 2021-05-09 17:54:58 UTC
-        println!("cargo:rustc-link-lib=static=bpf");
+        println!("cargo:rustc-link-lib=bpf");

This still should stay static I believe and depend on libbpf-static

Comment 5 Robert-André Mauchin 🐧 2021-05-09 18:43:58 UTC
I,ve changed this to:

diff -Naur a/bindings.h b/bindings.h
--- a/bindings.h	2021-01-19 11:12:47.000000000 -0800
+++ b/bindings.h	2021-05-09 10:40:12.043397710 -0700
@@ -1,8 +1,8 @@
-#include "libbpf/include/uapi/linux/if_link.h"
-#include "libbpf/src/bpf.h"
-#include "libbpf/src/btf.h"
-#include "libbpf/src/libbpf.h"
-#include "libbpf/src/xsk.h"
+#include <linux/if_link.h>
+#include <bpf/bpf.h>
+#include <bpf/btf.h>
+#include <bpf/libbpf.h>
+#include <bpf/xsk.h>
 
 extern __u64 *_xsk_ring_prod__fill_addr(struct xsk_ring_prod *fill, __u32 idx);
 
@@ -32,4 +32,4 @@
 
 extern __u64 _xsk_umem__extract_offset(__u64 addr);
 
-extern __u64 _xsk_umem__add_offset_to_addr(__u64 addr);
\ No newline at end of file
+extern __u64 _xsk_umem__add_offset_to_addr(__u64 addr);
diff -Naur a/build.rs b/build.rs
--- a/build.rs	2021-01-19 11:12:47.000000000 -0800
+++ b/build.rs	2021-05-09 10:41:00.968869985 -0700
@@ -10,39 +10,16 @@ fn main() {
     let out_dir_str = out_dir.to_str().unwrap();

     if cfg!(target_os = "linux") {
-        let status = Command::new("make")
-            .arg("install")
-            .env("BUILD_STATIC_ONLY", "y")
-            .env("PREFIX", "/")
-            .env("LIBDIR", "")
-            .env("OBJDIR", out_dir.join("obj").to_str().unwrap())
-            .env("DESTDIR", out_dir_str)
-            .env("CFLAGS", "-g -O2 -Werror -Wall -fPIC")
-            .current_dir(src_dir.join("libbpf/src"))
-            .status()
-            .unwrap();
-
-        assert!(status.success());
-
-        let status = Command::new("make")
-            .arg("clean")
-            .current_dir(src_dir.join("libbpf/src"))
-            .status()
-            .unwrap();
-
-        assert!(status.success());
-
         cc::Build::new()
             .file("bindings.c")
-            .include(src_dir.join("libbpf/include"))
-            .include(src_dir.join("libbpf/include/uapi"))
             .out_dir(out_dir_str)
             .compile("bindings");

         println!("cargo:rustc-link-search=native={}", out_dir_str);
         println!("cargo:rustc-link-lib=elf");
         println!("cargo:rustc-link-lib=z");
-        println!("cargo:rustc-link-lib=static=bpf");
+        println!("cargo:rustc-link-lib=static=bpf");
+        println!("cargo:rustc-link-search=native={}", "/usr/lib64");
         println!("cargo:include={}/include", out_dir_str);
     }
 }


and it works but it shouldn't be hard-coded to "/usr/lib64", Maybe passed through an environment variable?

Comment 6 Daniel Xu 2021-05-10 18:19:15 UTC
I've put up a PR to add a build flag to unbundle libbpf in libbpf-sys: https://github.com/alexforster/libbpf-sys/pull/18

Comment 7 Davide Cavalca 2021-05-10 18:57:20 UTC
Spec URL: https://dcavalca.fedorapeople.org/review/rust-libbpf-sys/rust-libbpf-sys.spec
SRPM URL: https://dcavalca.fedorapeople.org/review/rust-libbpf-sys/rust-libbpf-sys-0.3.0~1-3.fc35.src.rpm

Changelog:
- Backport PR#18 and turn the novendor feature on by default

Comment 9 Robert-André Mauchin 🐧 2021-05-11 16:01:26 UTC
No need to repeat this:

Requires:       kernel-headers
Requires:       elfutils-libelf-devel
Requires:       libbpf-devel
Requires:       zlib-devel

in %package     -n %{name}+default-devel*
as it already depends on -devel.


Package approved.

Comment 10 Davide Cavalca 2021-05-11 16:04:23 UTC
Thanks!

$ fedpkg request-repo rust-libbpf-sys 1958330
https://pagure.io/releng/fedora-scm-requests/issue/33925

Comment 11 Gwyn Ciesla 2021-05-11 16:19:44 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libbpf-sys

Comment 12 Fedora Update System 2021-05-11 22:07:58 UTC
FEDORA-2021-efb45c264a has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2021-efb45c264a

Comment 13 Fedora Update System 2021-05-12 07:05:48 UTC
FEDORA-2021-efb45c264a has been pushed to the Fedora 34 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-efb45c264a \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-efb45c264a

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 14 Fedora Update System 2021-05-20 01:11:39 UTC
FEDORA-2021-efb45c264a has been pushed to the Fedora 34 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.