Bug 1576879
Summary: | Review Request: ignition - First boot installer and configuration tool | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Steve Milner <smilner> |
Component: | Package Review | Assignee: | Neal Gompa <ngompa13> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | dustymabe, jlebon, miabbott, ngompa13, package-review, walters, zebob.m |
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: | 2018-09-08 14:43:04 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
Steve Milner
2018-05-10 15:31:58 UTC
Looking at https://fedoraproject.org/wiki/PackagingDrafts/Go and docker.spec, it looks like we should do: BuildRequires: %{?go_compiler:compiler(go-compiler)}%{!?go_compiler:golang >= 1.6.2} Maybe? There's also: ExclusiveArch: %{go_arches} Which it looks like docker is carrying a workaround that we could probably drop. Other than that, LGTM. Thanks Colin. I'll update, test, and upload changes to my Fedora People space. Updated and uploaded both spec and SRPM rebuild to https://smilner.fedorapeople.org/ignition/. [steve@qward SPECS]$ rpmlint /home/steve/rpmbuild/RPMS/x86_64/ignition-0.23.0-0.4.fc26.x86_64.rpm # not an ssue ignition.x86_64: W: spelling-error %description -l en_US systemd -> systems, system, system d # Not an issue ignition.x86_64: W: spelling-error %description -l en_US networkd -> networks, network, networked # Expected ignition.x86_64: W: unstripped-binary-or-object /usr/bin/ignition # Can raise with upstream if this is an issue, but I don't think this would stop inclusion ignition.x86_64: E: missing-call-to-chdir-with-chroot /usr/bin/ignition # Expected ignition.x86_64: W: unstripped-binary-or-object /usr/bin/ignition-validate # Expected ignition.x86_64: W: no-manual-page-for-binary ignition # Expected ignition.x86_64: W: no-manual-page-for-binary ignition-validate 1 packages and 0 specfiles checked; 1 errors, 6 warnings. [steve@qward SPECS]$ rpmlint ignition.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. I'm not sure of what's typical here so forgive my ignorance, but Ignition isn't particularly useful without an appropriate dracut configuration. Would the corresponding dracut module be something that should be included in the Ignition RPM, or would that typically be in a separate RPM? Derek, Most likely it would be seperate. It's possible that the ignition package would eventually require said dracut module/config. I think I agree with derek here. seems like ignition isn't that useful without the dracut modules so why not include them? In this case so that we can make the stream pull from dist-git to rebuild. If we need to make more modifications to the spec then I'll suggest we move towards direct building for the time being in the stream. I have a couple of questions here: 1. Why are we disabling debuginfo? That has been working with Go for several releases now. 2. My understanding is that ignition uses dracut modules and has dracut module configuration too. I don't see it in the ignition source tree, though. Where is it, and what will provide it? I would expect a dracut module subpackage, like how other packages do it (e.g. kiwi). I also think the changes that Colin requested in comment 1 need to be done as well. You also have an empty message entry in your %changelog. Please fix. :) - 0.24 is published. - Unbundle vendor, that might require lots of work packaging the missing deps. Use gofed to package these. %prep %autosetup -n %{name}-%{version} rm -rf vendor Discovering package dependencies Class: github.com/ajeddeloh/go-json (golang-github-ajeddeloh-go-json) PkgDB=False Class: github.com/aws/aws-sdk-go (golang-github-aws-aws-sdk-go) PkgDB=True Class: github.com/coreos/go-semver (golang-github-coreos-go-semver) PkgDB=True Class: github.com/coreos/go-systemd (golang-github-coreos-go-systemd) PkgDB=True Class: github.com/pin/tftp (golang-github-pin-tftp) PkgDB=False Class: github.com/sigma/vmw-guestinfo (golang-github-sigma-vmw-guestinfo) PkgDB=False Class: github.com/vincent-petithory/dataurl (golang-github-vincent-petithory-dataurl) PkgDB=False Class: github.com/vmware/vmw-ovflib (golang-github-vmware-vmw-ovflib) PkgDB=False Discovering test dependencies Class: github.com/stretchr/testify (golang-github-stretchr-testify) PkgDB=True - Consider using https://fedoraproject.org/wiki/More_Go_packaging . See examples: https://eclipseo.fedorapeople.org/golang/ - Thus I would not use the build script but a standard %build section like this: %build %gobuildroot %gobuild -o _bin/ignition %{goipath}/internal %gobuild -o _bin/ignition-validate" %{goipath}/validate - Unbundle vendor, that might require lots of work packaging the missing deps. Use gofed to package these. I don't think we have an interest in doing this right now. We have higher priority things to do: for example, fixing Ignition to work more nicely with SELinux. Colin, Agreed. (In reply to Colin Walters from comment #10) > - Unbundle vendor, that might require lots of work packaging the missing > deps. Use gofed to package these. > > I don't think we have an interest in doing this right now. We have higher > priority things to do: for example, fixing Ignition to work more nicely with > SELinux. If you're not going to unbundle (as I agree you should), then you need to do the right thing and document _all_ your bundled dependencies, per the policy: https://fedoraproject.org/wiki/Bundled_Libraries#Requirement_if_you_bundle Also, please properly post the Spec and Source RPM links every time you update the package. No one can properly review your package if you don't. yeah. will document bundled dependencies using gofed like I did for the kompose rpm when I originally created it: https://src.fedoraproject.org/rpms/kompose/c/abdc98fbde943502c205f3d410991f7081225baa?branch=master ok updated. ran it through gofed and tried to take notes about my steps. Here are my notes [1], the original spec generated by gofed [2], and the modified spec I edited [3]. I created a scratch build [4] and an srpm from that [5] can be accessed. [1] https://raw.githubusercontent.com/dustymabe/ignition-rpm/master/notes.txt [2] https://raw.githubusercontent.com/dustymabe/ignition-rpm/cd20621c8cfa1027f7afd76bac7af11899a453a2/ignition.spec.orig [3] https://raw.githubusercontent.com/dustymabe/ignition-rpm/cd20621c8cfa1027f7afd76bac7af11899a453a2/ignition.spec [4] https://koji.fedoraproject.org/koji/taskinfo?taskID=27790594 [5] https://kojipkgs.fedoraproject.org//work/tasks/595/27790595/ignition-0.26.0-0.1.git7610725.fc29.src.rpm Taking this review. it was requested that I add ignition-dracut as a subpackage. While I originally didn't want to do this because of spec file complexity I gave in (you're welcome Neal :)). Here is updated information spec [1] SRPM [2] scratch build [3] [1] https://raw.githubusercontent.com/dustymabe/ignition-rpm/3d83b5d89ce2613647a4eb6b21c1a6bc66b52ca7/ignition.spec [2] https://kojipkgs.fedoraproject.org//work/tasks/2449/27862449/ignition-0.26.0-0.1.git7610725.fc29.src.rpm [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=27862448 and to make fedora-review tool happy: Spec URL: https://raw.githubusercontent.com/dustymabe/ignition-rpm/3d83b5d89ce2613647a4eb6b21c1a6bc66b52ca7/ignition.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/2449/27862449/ignition-0.26.0-0.1.git7610725.fc29.src.rpm > %global dracutprovider_prefix %{provider}.%{provider_tld}/%{project}/%{repo}
> %global dracutimport_path %{provider_prefix}
fedora-review failed to validate the sources because the URL produced for Source1 was wrong. That was cuased by these two not being set to the correct macros. Please fix.
fixed. Spec URL: https://raw.githubusercontent.com/dustymabe/ignition-rpm/e60050652e3bf43c79ee0b69cfa969b824220131/ignition.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/3005/27863005/ignition-0.26.0-0.1.git7610725.fc29.src.rpm *** Bug 1594378 has been marked as a duplicate of this bug. *** Package review notes: [x] Package naming follows guidelines [x] Bundled components are correctly specified [x] Licensing is correctly documented [x] Macros are used consistently [x] No serious issues from rpmlint that aren't already being worked on [!] ignition-dracut does not do "Requires: %{name} = %{version}-%{release}" [!] Unnecessary "%defattr(-,root,root,0755)" in dracut subpackage Please fix the following issues noted above. updated: Spec URL: https://raw.githubusercontent.com/dustymabe/ignition-rpm/a789cdea4942165f80d04e2916d363605a39caa1/ignition.spec Looks good to me. PACKAGE APPROVED. Per request, I officially hand off the package in this review to Dusty Mabe. (fedrepo-req-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/ignition (fedrepo-req-admin): Processing with --force at the request of @mohanboddu |