Bug 2188762

Summary: Review Request: golang-github-containerd-cgroups-3 - Cgroups package for Go
Product: [Fedora] Fedora Reporter: Sergio Basto <sergio>
Component: Package ReviewAssignee: Robert-André Mauchin 🐧 <zebob.m>
Status: ASSIGNED --- QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: mikel, package-review, zebob.m
Target Milestone: ---Flags: zebob.m: fedora-review?
zebob.m: needinfo? (sergio)
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Sergio Basto 2023-04-22 03:58:51 UTC
Spec URL: https://sergiomb.fedorapeople.org/golang-github-containerd-cgroups-3/golang-github-containerd-cgroups-3.spec
SRPM URL: https://sergiomb.fedorapeople.org/golang-github-containerd-cgroups-3/golang-github-containerd-cgroups-3-3.0.1-1.fc39.src.rpm

Description:
Go package for creating, managing, inspecting, and destroying cgroups.

Fedora Account System Username: sergiomb

Comment 1 Sergio Basto 2023-04-22 03:58:54 UTC
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=100241683

Comment 2 Mikel Olasagasti Uranga 2023-05-18 18:35:04 UTC
> Epoch: 1

Why epoch is needed for this new package?

> Release: 3%{?dist}

Why it doesn't start from 1? 

But also, why are you not using %autorelease?

> %build
> mkdir v3
> mv cmd/go.mod cmd/go.sum v3
> for cmd in cmd/* ; do
>   %gobuild -o %{gobuilddir}/bin/$(basename $cmd) %{goipath}/$cmd
> done
(...)
> install -m 0755 -vd                     %{buildroot}%{_bindir}
> install -m 0755 -vp %{gobuilddir}/bin/* %{buildroot}%{_bindir}/

Is the cmd useful?

Comment 3 Sergio Basto 2023-05-18 22:15:31 UTC
(In reply to Mikel Olasagasti Uranga from comment #2)
> > Epoch: 1
> 
> Why epoch is needed for this new package?
> 

because golang-github-containerd-cgroups also have epoch = 1  and golang-github-containerd-cgroups-3 need be bigger than golang-github-containerd-cgroups 


> > Release: 3%{?dist}
> 
> Why it doesn't start from 1? 
> 
> But also, why are you not using %autorelease?
> 

fixed 

> > %build
> > mkdir v3
> > mv cmd/go.mod cmd/go.sum v3
> > for cmd in cmd/* ; do
> >   %gobuild -o %{gobuilddir}/bin/$(basename $cmd) %{goipath}/$cmd
> > done
> (...)
> > install -m 0755 -vd                     %{buildroot}%{_bindir}
> > install -m 0755 -vp %{gobuilddir}/bin/* %{buildroot}%{_bindir}/
> 
> Is the cmd useful?

if we can build it and provides the binary /usr/bin/cgctl , why not ? 
golang-github-containerd-cgroups doesn't provides it, but we need a trick to build it, we need to move cmd/go.mod cmd/go.sum away from the directory cmd [1]


[1]
mkdir v3
mv cmd/go.mod cmd/go.sum v3

Comment 4 Mikel Olasagasti Uranga 2023-05-19 17:04:11 UTC
>> Why epoch is needed for this new package?
> because golang-github-containerd-cgroups also have epoch = 1  and golang-github-containerd-cgroups-3 need be bigger than golang-github-containerd-cgroups 

In the case of `golang-github-containerd-cgroups` it is required because a newer version was pushed and then restored the old one.

For `golang-github-containerd-cgroups-3`, the package name is different (has the -3) and goipath is also different. I don't see it as required.

> %global goipathsex0 github.com/containerd/cgroups/v3/cmd

Why is this one required? Builds fine for me without it.

> # Needs special permissions for cgroupsv2
> %bcond_with check

Skip those tests failing, not all the tests. You can check how `doctl.spec` does for example to skip just some tests.

> RPM build warnings:
>    File listed twice: /usr/share/doc/golang-github-containerd-cgroups-3/metrics.pb.txt
>    File listed twice: /usr/share/doc/golang-github-containerd-cgroups-3-devel/metrics.pb.txt

You can check how to fix this one using `golang-github-cloudflare-circl.spec` LICENSE rename as example.

Comment 5 Sergio Basto 2023-05-19 19:34:24 UTC
- I removed the Epoch

- enabled teste and removed "TestCgroupv2CpuStats" "TestSystemdCgroupCpuController" "TestSystemdCgroupCpuController_NilWeight" "TestEventChanCleanupOnCgroupRemoval"\
"TestCgroupv2MemoryStats" "TestSystemdCgroupMemoryController" "TestCgroupv2PidsStats" "TestSystemdCgroupPidsController" "TestKill" because needs special permissions 

- Fixed the File listed twice like was in golang-github-cloudflare-circl.spec


about goipathsex0 in golang channel me and gotmax23 talk: 

sergiomb: the problem of golang-github-containerd-cgroups-3 is have cmd/go.mod  cmd/go.sum  which aren't meant to bulld
  mkdir v3
  mv cmd/go.mod cmd/go.sum v3
  seems thats fixes the problem 
gotmax23G :That won't work either
sergiomb: can you explain ? where it will fail
gotmax23: The cmd directory needs to be outside of the v3 directory.

(later)

gotmax23: Hmm, it might be possible to regenerate a clean specfile with go2rpm --forge https://github.com/containerd/cgroups github.com/containerd/cgroups/v3 and then add %global goipathsex0 github.com/containerd/cgroups/v3/cmd right under %gometa.

(later) 

sergiomb: after %gometa %global goipathsex0 github.com/containerd/cgroups/v3/cmd  makes disappear cmd at all
gotmax23: That's the point. Since no other package should depend on the cmd module, it might be best to just exclude it rather than install it at the wrong location.


new spec and src.rpm uploaded

Comment 6 Robert-André Mauchin 🐧 2023-08-14 16:25:52 UTC
I have my own SPEC going on in #2229425.


Anyhow

 - Bump to 3.0.2
 - Are these files needed? For what purpose are they in docs?

cgroup2-stats-metrics.pb.txt cgroup1-stats-metrics.pb.txt

 - What is the purpose of:

mkdir v3
mv cmd/go.mod cmd/go.sum v3


Just do:

%build
%gobuild -o %{gobuilddir}/bin/cgctl %{goipath}/cmd/cgctl

 - Format this:

for test in "TestCgroupv2CpuStats" \
            "TestSystemdCgroupCpuController" \
            "TestSystemdCgroupCpuController_NilWeight" \
            "TestEventChanCleanupOnCgroupRemoval" \
            "TestCgroupv2MemoryStats" \
            "TestSystemdCgroupMemoryController" \
            "TestCgroupv2PidsStats" \
            "TestSystemdCgroupPidsController" \
            "TestKill" \
            "TestMoveTo" ; do
    awk -i inplace '/^func.*'"$test"'\(/ { print; print "\tt.Skip(\"disabled failing test\")"; next}1' $(grep -rl $test)
done

 - Don't glob the binary directory:

%files
%license LICENSE
%doc README.md
%{_bindir}/cgctl

 - > sergiomb: the problem of golang-github-containerd-cgroups-3 is have cmd/go.mod  cmd/go.sum  which aren't meant to bulld


This is really not needed, Fedora Go macros don't use Go modules. You can remove it.


Please update the SPEC and we'll get back to it.

Comment 7 Robert-André Mauchin 🐧 2023-08-14 16:26:43 UTC
*** Bug 2229425 has been marked as a duplicate of this bug. ***