We currently don't have a sponsor. This is our first package.

Taking this review.

(In reply to Matthew Yee from comment #0)
> Spec URL:
> https://github.com/sustainable-computing-io/kepler/blob/main/packaging/rpm/
> kepler.spec
> SRPM URL: N/A
> Description: Kepler (Kubernetes-based Efficient Power Level Exporter) uses
> eBPF to probe performance counters and other system stats, use ML models to
> estimate workload energy consumption based on these stats, and exports them
> as Prometheus metrics
> Fedora Account System Username: myee111

Please provide a spec file and an SRPM from URLs that can be curled so that fedora-review can fetch them.

Hey Neal

This URL should work:
https://raw.githubusercontent.com/sustainable-computing-io/kepler/main/packaging/rpm/kepler.spec

The RPM is available here https://github.com/sustainable-computing-io/kepler/releases/tag/v0.7.3

@Neal, anything you need please let us know asap, thanks

Spec review:

> %undefine _disable_source_fetch

This is not allowed. The Fedora build system does not provide internet access.

> Version:        %{getenv:_VERSION_}
> Release:        %{getenv:_RELEASE_}

This is not allowed. These values need to be static and managed from the spec file.

> BuildArch:      %{getenv:_ARCH_}

Ignoring the fact that this form is not allowed, this makes no sense and should be dropped. The build system will build for all architectures.

> License:        Apache License 2.0

This is not valid. This needs to be an SPDX license identifier tag.

Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_valid_license_short_names

> BuildRequires: systemd

I don't see a reason to require systemd when you only need the RPM macros. Instead, require "systemd-rpm-macros".

> BuildRequires: clang llvm llvm-devel zlib-devel make libbpf

Please format these such that each build dependency on its own line.

> Requires:       elfutils-libelf-devel

Are you sure you need the devel package at runtime?

> %ifarch x86_64
> GOARCH=amd64
> %endif

You're going to need aliases for all RPM arches in Fedora to map to Go arches.

> make genlibbpf _build_local GOOS=${GOOS} GOARCH=${GOARCH} ATTACHER_TAG=libbpf

The Go compilation build flags need to be passed in.

> install -d %{buildroot}/var/lib/kepler/data
> install -d %{buildroot}/var/lib/kepler/bpfassets
> install -d %{buildroot}/etc/kepler/kepler.config

"/var/lib" should be "%{_sharedstatedir}" and "/etc" should be "%{_sysconfdir}".

> install -p -m644 ./data/cpus.yaml %{buildroot}/var/lib/kepler/data/cpus.yaml
> install -p -m644 ./data/model_weight/acpi_AbsPowerModel.json %{buildroot}/var/lib/kepler/data/acpi_AbsPowerModel.json
> install -p -m644 ./data/model_weight/acpi_DynPowerModel.json %{buildroot}/var/lib/kepler/data/acpi_DynPowerModel.json
> install -p -m644 ./data/model_weight/intel_rapl_AbsPowerModel.json %{buildroot}/var/lib/kepler/data/intel_rapl_AbsPowerModel.json
> install -p -m644 ./data/model_weight/intel_rapl_DynPowerModel.json %{buildroot}/var/lib/kepler/data/intel_rapl_DynPowerModel.json

Are you sure this should be in "%{_sharedstatedir}"? It seems like these are static files and should be installed into "%{_datadir}".

> %post
>
> %systemd_post kepler.service

This is incomplete. 

Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_systemd

> /var/lib/kepler/bpfassets/amd64_kepler.bpf.o

This should probably be installed into "%{_libexecdir}/kepler/bpfassets" instead.

Also one more thing...

> Source0:        https://github.com/sustainable-computing-io/kepler/archive/refs/tags/%{version}.tar.gz

This should be "%{url}/archive/%{version}/%{name}-%{version}.tar.gz".

> install -p -m644 ./bpfassets/libbpf/bpf.o/amd64_kepler.bpf.o %{buildroot}/var/lib/kepler/bpfassets/amd64_kepler.bpf.o

Is this amd64_kepler.bpf.o binary generated during the build or are we using the one from the git repo/tarball here?

For official Fedora builds, it will need to be built as part of the rpmbuild process (binary blobs are not allowed) and  that will result in a binary of the build system architecture (which is not necessarily x86_64).

| The build system will build for all architectures.

What I suspect is actually needed here is the intersection of 'all support golang architectures' and 'all supported bpf architectures' (which is not all architectures, it turns out).  There's a similar situation in the Grafana build, where we need golang and nodejs architectures only, and this spec file snippet is used there:

%global grafana_arches %{lua: go_arches = {}
  for arch in rpm.expand("%{go_arches}"):gmatch("%S+") do
    go_arches[arch] = 1
  end
  for arch in rpm.expand("%{nodejs_arches}"):gmatch("%S+") do
    if go_arches[arch] then
      print(arch .. " ")
  end
end}

However, I'm not aware of a bpf_arches macro - for PCP we are using a local macro defined as: 'x86_64 %{power64} aarch64 s390x'.

Thanks Neal! Will post a new spec for the Fedora requirements.

(In reply to Nathan Scott from comment #9)
> > install -p -m644 ./bpfassets/libbpf/bpf.o/amd64_kepler.bpf.o %{buildroot}/var/lib/kepler/bpfassets/amd64_kepler.bpf.o
> 
> Is this amd64_kepler.bpf.o binary generated during the build or are we using
> the one from the git repo/tarball here?
> 
> For official Fedora builds, it will need to be built as part of the rpmbuild
> process (binary blobs are not allowed) and  that will result in a binary of
> the build system architecture (which is not necessarily x86_64).

eBPF binaries, regardless of arch, apparently look like x86_64 ELF blobs, which means the only way to distinguish them is by name...

From the kepler git repo https://github.com/sustainable-computing-io/kepler.git ...

$ file ./bpfassets/libbpf/bpf.o/amd64_kepler.bpf.o 
./bpfassets/libbpf/bpf.o/amd64_kepler.bpf.o: ELF 64-bit LSB relocatable, eBPF, version 1 (SYSV), with debug_info, not stripped

My question remains - is this built during the rpmbuild process or is it pulled from the tarball?  I believe it needs to be built locally as part of the Fedora build each time - 
https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be-packaged/#prebuilt-binaries-or-libraries

New question :) - if this is platform-independent eBPF bytecode, why prefix the name with amd64?  For people on other arches this is going to be a source of confusion - looking at my aarch64 laptop and my confusion RN for example ;)

Relevant excerpt from the packaging guidelines (link above):

| When you encounter prebuilt binaries in a package you MUST:
| 
|    Remove all pre-built program binaries and program libraries in %prep prior to the building of the package. Examples include, but are not limited to, *.class, *.dll, *.DS_Store, *.exe, *.jar, *.o, *.pyc, *.pyo, *.egg, *.so, *.swf files.
| 
|    Ask upstream to remove the binaries in their next release.

so kepler.spec needs a %prep section that removes ./bpfassets/libbpf/bpf.o/amd64_kepler.bpf.o prior to the build commencing.

From specfile:

> BuildRequires: [...] libbpf

How does Kepler load the BPF program? I assume it uses libbpf so libbpf should be a runtime dependency (i.e. in "Requires"), unless you're linking against it statically.

On the contrary, I'd expect libbpf-devel in "BuildRequires".

> New question :) - if this is platform-independent eBPF bytecode, why prefix the name with amd64?  For people on other arches this is going to be a source of confusion - looking at my aarch64 laptop and my confusion RN for example ;)

Agreed with Nathan here. BPF binaries are arch-independent (they are ELFs with a special "BPF" architecture) so the prefix shouldn't be there.

>  I believe it needs to be built locally as part of the Fedora build each time

Despite being arch-independent, I agree that the binary should be rebuilt as a part of the specfile, as per Fedora guidelines.
In fact, IIUC, that's exactly what

> make genlibbpf _build_local GOOS=${GOOS} GOARCH=${GOARCH} ATTACHER_TAG=libbpf

in the specfile does. So you just need to remove ./bpfassets/libbpf/bpf.o/amd64_kepler.bpf.o in the %prep phase, as mentioned above.

(In reply to Neal Gompa from comment #7)
> > /var/lib/kepler/bpfassets/amd64_kepler.bpf.o
> 
> This should probably be installed into "%{_libexecdir}/kepler/bpfassets"
> instead.

Actually, we've been trying to establish %{_libdir}/bpf/ as the place to put BPF object files, cf: https://src.fedoraproject.org/rpms/xdp-tools/blob/rawhide/f/xdp-tools.spec#_105

However, it looks like the loader code hard-codes the location of the object file: https://github.com/sustainable-computing-io/kepler/blob/main/pkg/bpfassets/attacher/libbpf_attacher.go#L41

This should probably be changed so the Makefile can override it.

(In reply to Viktor Malik from comment #15)
> From specfile:
> 
> > BuildRequires: [...] libbpf
> 
> How does Kepler load the BPF program? I assume it uses libbpf so libbpf
> should be a runtime dependency (i.e. in "Requires"), unless you're linking
> against it statically.
> 
> On the contrary, I'd expect libbpf-devel in "BuildRequires".

Looking at the top-level Makefile, it does appear that Kepler links statically against libbpf: https://github.com/sustainable-computing-io/kepler/blob/main/Makefile#L51

Not sure if we have a policy for this in Fedora, but at least in RHEL this won't be allowed. So at some point this needs to be changed to link dynamically against the system libbpf.

> > New question :) - if this is platform-independent eBPF bytecode, why prefix the name with amd64?  For people on other arches this is going to be a source of confusion - looking at my aarch64 laptop and my confusion RN for example ;)
> 
> Agreed with Nathan here. BPF binaries are arch-independent (they are ELFs
> with a special "BPF" architecture) so the prefix shouldn't be there.

AFAICT from the Kepler sources, it's arch-specific because the build includes different versions of vmlinux.h depending on the defined architecture: https://github.com/sustainable-computing-io/kepler/blob/main/bpfassets/libbpf/src/kepler.bpf.h

This should not be necessary for most accesses, CO-RE can be used instead. I am not sure if there are some arch-specific structs that Kepler needs to access, though, and whether CO-RE can handle those.

(In reply to Toke Høiland-Jørgensen from comment #16)
> 
> Looking at the top-level Makefile, it does appear that Kepler links
> statically against libbpf:
> https://github.com/sustainable-computing-io/kepler/blob/main/Makefile#L51
> 
> Not sure if we have a policy for this in Fedora, but at least in RHEL this
> won't be allowed. So at some point this needs to be changed to link
> dynamically against the system libbpf.

In Fedora, it is very much discouraged:

https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling

Won't do.