Bug 2361600 (igvm, libigvm)

Cannot find any valid SRPM URL for this ticket. Common causes are:

- You didn't specify `SRPM URL: ...` in the ticket description
  or any of your comments
- The URL schema isn't HTTP or HTTPS
- The SRPM package linked in your URL doesn't match the package name specified
  in the ticket summary


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

[fedora-review-service-build]

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8951388
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2361600-libigvm/fedora-rawhide-x86_64/08951388-libigvm/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

I think my original suggestion to call this `libigvm` is wrong, sorry. yeah a libigvm.a is being installed, but the .pc file is `igvm.pc` and the upstream repo is called `igvm`. So I think this package should be called `igvm`.

igvm here is _only_ a static library. So the package layout is a bit interesting. With libs like glib2, pcre2, glibc you'll have package hierarchy like

glib2 (or some packages call this NAME-libs)
  files: lib/*.so.*

glib2-devel
  files: lib/*.so , pkgconfig, /usr/include
  Requires: glib2

glib2-static
  files: lib/*.a
  Requires: glib2-devel

But since libigvm doesn't have .so files at all, `libigvm` doesn't exist, and `libigvm-devel` on its own is pointless.
So I _think_ this should stuff pkgconfig and /usr/include bits into libigvm-static, and kill the libigvm-devel package too.
If something changed and libigvm ever becomes dynamically linkable as well, the package can switch to the above ideal layout easypeasy.

@berrange can you sanity check me here, does all that sound reasonable?

Some other bits:

+ if the dump_igvm tool is just a helper tool, I think it should go in a `NAME-tools` package

+ rust-bitfield-struct is at version 0.11 in Fedora, so you need to adjust your version patch. I confirmed it's still building with that 

+ take a look at /usr/lib/rpm/macros.d/macros.cargo %__cargo macro. I think you want to export `CARGO_HOME=.cargo` and `RUSTFLAGS='%{build_rustflags}'` too so that trickles down to cargo called by the Makefile.

> But since libigvm doesn't have .so files at all, `libigvm` doesn't exist, and `libigvm-devel` on its own is pointless.
> So I _think_ this should stuff pkgconfig and /usr/include bits into libigvm-static, and kill the libigvm-devel package too.

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

Says that for the "static only" case don't create a -static, put the .a file in the -devel

+ if the dump_igvm tool is just a helper tool, I think it should go in a `NAME-tools` package

Agreed

> Spec URL: https://gitlab.com/luigileonardi/libigvm/-/raw/no_vendor/libigvm.spec
> SRPM URL: https://download.copr.fedorainfracloud.org/results/lleonard/sev-snp-coconut/fedora-rawhide-x86_64/08918482-libigvm/libigvm-0.3.4-1.fc43.src.rpm

The src.rpm has gone, and the only build I see in copr has a spec file that is different from the above one, so I'm unclear as to what this ticket should be reviewing as the "latest". 

> export EXTRA_PARAMS="--profile rpm -Z avoid-dev-deps"

What does the 'avoid-dev-deps' flag skip ?


> # Run test just when necessary. This is not upstream
> Patch6: 0007-Remove-test-from-all-target.patch

Seems like upstream should define a top level target 'build' and have 'all' depend on 'build'  + 'test'.


> %if %{with check}

Is that needed ? If building locally you can give an arg to RPM to skip checks. If building in Fedora I'd expects checks to always be run & passing

Hi Cole and Daniel,

> So I think this package should be called `igvm`.
Should I create a new ticket or just rename this one?

> rust-bitfield-struct is at version 0.11 in Fedora, so you need to adjust your version patch. I confirmed it's still building with that 
good catch, thanks for checking!

> take a look at /usr/lib/rpm/macros.d/macros.cargo %__cargo macro. I think you want to export `CARGO_HOME=.cargo` and `RUSTFLAGS='%{build_rustflags}'` too so that trickles down to cargo called by the Makefile.
good point, will do!

> What does the 'avoid-dev-deps' flag skip ?
It's a flag it's normally used by %cargo_build (it's in __cargo_common_opts) but yes, it's useless in this case since there are no dev-dependencies. I can drop RUSTC_BOOTSTRAP too.

> Is that needed ? If building locally you can give an arg to RPM to skip checks. If building in Fedora I'd expects checks to always be run & passing
It's from the default cargo2rpm template but I can drop it.

> The src.rpm has gone, and the only build I see in copr has a spec file that is different from the above one, so I'm unclear as to what this ticket should be reviewing as the "latest". 
Where can I upload the file so that it's not removed after a while?

> Seems like upstream should define a top level target 'build' and have 'all' depend on 'build'  + 'test'.
I'll send a PR upstream

So to recap, I need to create a `igvm-devel` package that contains .a and headers etc and a `igmm-tool` just with the `dump_igvm`?

Thanks!

> Where can I upload the file so that it's not removed after a while?

Typically you can use fedorapeople.org https://fedoraproject.org/wiki/Infrastructure/fedorapeople.org#Accessing_your_fedorapeople.org_space

Spec URL: https://lleonard.fedorapeople.org/igvm.spec
SRPM URL: https://lleonard.fedorapeople.org/igvm-0.3.4-1.fc42.src.rpm

changes: 
- Renamed to igvm, moved .a and igvm.pc to devel
- Created -tools package that contains dump_igvm
- Makefile target change has been merged upstream.
- exported cargo macros and updated flags
- bumped rust-bitfield-struct to v0.11

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9100128
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2361600-igvm/fedora-rawhide-x86_64/09100128-igvm/fedora-review/review.txt

Found issues:

- Package has .a files: igvm-devel. Does not provide -static: igvm-devel.
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Spec URL: https://lleonard.fedorapeople.org/igvm.spec
SRPM URL: https://lleonard.fedorapeople.org/igvm-0.3.4-2.fc42.src.rpm

changes:
- Adding Provides for the -static package.

I don't think we need `Requires: %{name} = %{version}-%{release}` for `devel` and `tools` since the `igvm` package is not created. WDYT?

Created attachment 2092116 [details]
The .spec file difference from Copr build 9100128 to 9100453

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9100453
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2361600-igvm/fedora-rawhide-x86_64/09100453-igvm/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

> I don't think we need `Requires: %{name} = %{version}-%{release}` for
> `devel` and `tools` since the `igvm` package is not created. WDYT?

Yeah that's fine.

I tried a scratch build, and i686 failed, the .a file wasn't generated.
Not worth figuring out, just add `ExcludeArch: %{ix86}"

rpmlint points out some issues with dump_igvm:

igvm-tools.x86_64: W: unused-direct-shlib-dependency /usr/bin/dump_igvm /lib64/libm.so.6
igvm-tools.x86_64: W: unstripped-binary-or-object /usr/bin/dump_igvm
igvm-tools.x86_64: W: position-independent-executable-suggested /usr/bin/dump_igvm

I _think_ these are all due to igvm_c/Makefile ignoring CFLAGS and friends when
building dump_igvm. rpm exports CFLAGS at beginning of %build/%install/%check
phases but the Makefile isn't adhering to them.

Is dump_igvm useful? The code is in a `sample/` directory upstream so not
sure if it's just a demo app or not. Maybe it's simpler to delete it from
packaging.

After those bits I think everything is good to go

Spec URL: https://lleonard.fedorapeople.org/igvm.spec
SRPM URL: https://lleonard.fedorapeople.org/igvm-0.3.4-3.fc42.src.rpm

> Not worth figuring out, just add `ExcludeArch: %{ix86}"
done!

> Is dump_igvm useful?
It is kind of useful, so I troubleshooted and fixed it:
I opened an upstream PR to add CFLAGS and LDFLAGS into the Makefile
I also had to remove %global debug_package %{nil}, in this way the binaries
are now stripped.

changes:
- Added ExcludeArch: %{ix86}
- Applied an upstream patch for the makefile
- Added README for -tools package

Thanks!

Created attachment 2093326 [details]
The .spec file difference from Copr build 9100453 to 9140866

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9140866
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2361600-igvm/fedora-rawhide-x86_64/09140866-igvm/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

LGTM now! setting fedora-review+

The Pagure repository was created at https://src.fedoraproject.org/rpms/igvm

FEDORA-2025-1d89eb2177 (igvm-0.3.4-1.fc43) has been submitted as an update to Fedora 43.
https://bodhi.fedoraproject.org/updates/FEDORA-2025-1d89eb2177

FEDORA-2025-1d89eb2177 (igvm-0.3.4-1.fc43) has been pushed to the Fedora 43 stable repository.
If problem still persists, please make note of it in this bug report.