Spec URL: https://fedorapeople.org/~bgilbert/fedora-coreos-config-transpiler.spec SRPM URL: https://fedorapeople.org/~bgilbert/fedora-coreos-config-transpiler-0.4.0-1.fc31.src.rpm Description: The Fedora CoreOS Config Transpiler (FCCT) translates human-readable Fedora CoreOS Configs (FCCs) into machine-readable Ignition configs for provisioning Fedora CoreOS machines. Fedora Account System Username: bgilbert There are a couple regrettable things here: - Dependency bundling. golang--. - The -nonlinux subpackage. This is needed for the current method of getting release binaries signed with Fedora keys: https://pagure.io/releng/issue/9057 Both of these are similar to how the (related) ignition package works.
Taking this review.
First pass through: > GOOS=windows %gocrossbuild -o ./fcct-windows internal/main.go Umm, is this 32-bit or 64-bit Windows? If it's for 64-bit Windows, it should probably be indicated as such. Technically, I have the same quibble with the Darwin build, but I think it's a lot less likely that would be confusing... :) > install -d -p %{buildroot}%{_libdir}/fcct Two things: 1. Why are we installing random binaries into %{_libdir}/fcct instead of %{_libexecdir}/fcct? 2. This directory is not correctly owned by any package generated by this spec. Please fix that.
Thanks for the review! > Umm, is this 32-bit or 64-bit Windows? If it's for 64-bit Windows, it should probably be indicated as such. The binaries match the package architecture, so yeah, 64-bit in both cases. The filenames ultimately uploaded to GitHub do include the CPU architecture, but it didn't seem necessary here. > 1. Why are we installing random binaries into %{_libdir}/fcct instead of %{_libexecdir}/fcct? They're not executable programs from Linux's perspective, so libexecdir didn't seem appropriate. libdir seemed a better fit for architecture-specific data. I've now removed the executable bits as well. > 2. This directory is not correctly owned by any package generated by this spec. Please fix that. Yup, fixed.
(In reply to Benjamin Gilbert from comment #3) > 1. Why are we installing random binaries into %{_libdir}/fcct instead of %{_libexecdir}/fcct? > > They're not executable programs from Linux's perspective, so libexecdir > didn't seem appropriate. libdir seemed a better fit for > architecture-specific data. I've now removed the executable bits as well. Then perhaps you should treat them as firmware, i.e under $datadir and in a noarch subpackage
Okay, I've made the -nonlinux subpackage noarch, moved its binaries to %{_datadir}/fcct, and added the CPU architecture to their filenames.
(In reply to Benjamin Gilbert from comment #5) > Okay, I've made the -nonlinux subpackage noarch, moved its binaries to > %{_datadir}/fcct, and added the CPU architecture to their filenames. Where's the new spec and SRPM?
(In reply to Neal Gompa from comment #6) > Where's the new spec and SRPM? ...uploaded to the wrong directory. Fixed now.
Package review notes: * Package conforms to the Fedora Packaging Guidelines * Package conforms to Go Packaging Guidelines (go2rpm-generated spec) * Package builds and installs properly * Package licensing is marked and license files are installed properly * No serious issues from rpmlint PACKAGE APPROVED.
My FAS ID is bgilbert and I confirm that I am the same person as the reporter of this bug.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/fedora-coreos-config-transpiler
Thanks for the review, Neal!