Bug 1794832
| Summary: | Review Request: fedora-coreos-config-transpiler - Fedora CoreOS Config Transpiler | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Benjamin Gilbert <bgilbert> |
| Component: | Package Review | Assignee: | Neal Gompa <ngompa13> |
| Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | behoward, jlebon, kparal, miabbott, ngompa13, package-review, yaneti |
| Target Milestone: | --- | Flags: | ngompa13:
fedora-review+
|
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2020-02-01 06:18:05 UTC | 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
Benjamin Gilbert
2020-01-24 19:55:05 UTC
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! |