Bug 331631
Summary: | Review Request: mknbi - Utility for creating network bootable images | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Eric Harrison <eharrison> | ||||
Component: | Package Review | Assignee: | Patrice Dumas <pertusus> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | chris.ricker, fedora-package-review, notting, wtogami | ||||
Target Milestone: | --- | Flags: | pertusus:
fedora-review+
wtogami: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2007-11-14 16:37: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: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 188611 | ||||||
Attachments: |
|
Description
Eric Harrison
2007-10-15 02:28:14 UTC
The "slight funkiness" could be problematic, because this package will only appear in the i386 repository due to the presence of ExclusiveArch:. I don't know if that's an issue for you. syslinux builds on x86_64 just fine and of course isn't applicable on ppc, sparc or alpha in any case. At first glance, %{_libdir}/mknbi looks to be unowned. Why not just have that directory in your %files section instead of listing all of the files underneath it separately? Honestly, barring some other reason for listing everything out, your %files section could be as simple as: %files %{_libdir}/mknbi/ %{_bindir}* %{_mandir}/man1/* %doc COPYING LOG README spec.txt but of course that's up to you. Regardless of what you do, you must still own %{_libdir}/mknbi. Yes, that is a problem. Ideally this package would be available on all arches. It builds boot images for i386 diskless clients, it is a quite reasonable use-case to want to build an boot image for an i386 client from a sparc server, for example. Any suggestions on getting the fedora build system to generate i386 assembly code within a noarch package? As for the *spec, it originated from upstream (http://downloads.sourceforge.net/etherboot/mknbi-1.4.4-1.src.rpm). It passed rpmlint, so I thought I'd just leave it. Might as well clean it up while fixing %{_libdir}/mknbi... directory ownership fixed & *spec have been cleaned up. I pulled the exclusivearch & tried building it on a x86_64 box w/o joy: first32.c:1: error: CPU you selected does not support x86-64 instruction set make: *** [first32.o] Error 1 Fedora packaging recommends using $RPM_OPT_FLAGS, but as far I can see, the Makefile ignores it and forces usage of specific values for CFLAGS. Is it intentional / needed ? This breaks compiling with the exact error you have seen. What does this do that network boot support in mkinitrd doesn't? It makes images that can be booted by etherboot or similar - rom boot loaders embedded on the NIC The NBI images are a container wrapping the vmlinuz/initramfs generated by mkinitrd. PXE can boot the files generated by mkinitrd, but Etherboot/Netboot/etc need some extra glue (and can only tftpboot 1 file, you cannot fetch separate kernel & initramfs files). http://togami.com/~warren/fedora/mknbi-1.4.4-5.fc8.src.rpm * Fri Oct 19 2007 Warren Togami <wtogami> 1.4.4-5 - Not noarch - Fix build and make ExclusiveArch: i386 x86_64 - more cleanups This builds i386 binaries within the x86_64 package, similar to how we package memtest86+. noarch was not a good choice for this package because it clearly contained architecture specific bytecode, and wouldn't run at all on PPC for example. rpmlint mknbi-1.4.4-5.fc8.i386.rpm mknbi.i386: E: no-binary mknbi.i386: E: only-non-binary-in-usr-lib No idea what these errors are about. Any ideas? [builder1@newcaprica SRPMS]$ rpmlint mknbi-1.4.4-5.fc8.src.rpm mknbi.src:30: W: rpm-buildroot-usage %build make BUILD_ROOT="$RPM_BUILD_ROOT" PREFIX=%{_prefix} Not harmful... mknbi.src:43: E: hardcoded-library-path in /usr/lib/mknbi/ This is necessary due to i386 binaries in the x86_64 package. Any objections to approving this? Any other suggestions for improvement? Regarding $RPM_OPT_FLAGS, we shouldn't use it here. This is essentially boot loader code, and we want to avoid surprises. It also isn't helpful to have a debuginfo package, because how are you going to debug it? (In reply to comment #8) Maybe %attr(-,root,root) /usr/lib/mknbi/ could be instead %{_prefix}/lib/mknbi/ > [builder1@newcaprica SRPMS]$ rpmlint mknbi-1.4.4-5.fc8.src.rpm > mknbi.src:30: W: rpm-buildroot-usage %build make BUILD_ROOT="$RPM_BUILD_ROOT" > PREFIX=%{_prefix} > > Not harmful... This should be fixed. Also you could do something along make BUILD_ROOT="$RPM_BUILD_ROOT" PREFIX=%{_prefix} \ RPMVERSION="$RPM_PACKAGE_VERSION" INSTALL='install -p' install to keep timestamps. (In reply to comment #9) > Regarding $RPM_OPT_FLAGS, we shouldn't use it here. This is essentially boot > loader code, and we want to avoid surprises. It also isn't helpful to have a > debuginfo package, because how are you going to debug it? i don't see why the normal $RPM_OPT_FLAGS shouldn't be used. For the debuginfo, isn't it possible to use some virtualization to debug? I suggest using %defattr(-,root,root,-) instead of %defattr(-,root,root) > i don't see why the normal $RPM_OPT_FLAGS shouldn't be used.
> For the debuginfo, isn't it possible to use some virtualization
> to debug?
Just to be sure I asked about this earlier today. Think about it: During the
boot loader the filesystem with debuginfo isn't even mounted yet. We would have
to ship unstripped binaries for any chance of debugging in qemu or something,
but eve then it is doubtful that it would work. If somebody really wants to
debug mknbi, it should be done with a custom build.
http://togami.com/~warren/fedora/mknbi.spec http://togami.com/~warren/fedora/mknbi-1.4.4-6.fc8.src.rpm This should be everything requested. perl should be in BuildRequires now. Why is LD used for linking instead of gcc? Also in my opinion it would be cleaner to use, on the make install line MANDIR=$RPM_BUILD_ROOT%{_mandir}/man1 BINDIR=$RPM_BUILD_ROOT%{_bindir} %install
rm -rf $RPM_BUILD_ROOT
make BUILD_ROOT="$RPM_BUILD_ROOT" PREFIX=%{_prefix} \
MANDIR=$RPM_BUILD_ROOT%{_mandir}/man1 \
BINDIR=$RPM_BUILD_ROOT%{_bindir} \
RPMVERSION="$RPM_PACKAGE_VERSION" INSTALL='install -p' install
You mean like this?
> Why is LD used for linking instead of gcc?
I don't know, but does it really matter? This was upstream's decision.
(In reply to comment #15) > %install > rm -rf $RPM_BUILD_ROOT > make BUILD_ROOT="$RPM_BUILD_ROOT" PREFIX=%{_prefix} \ > MANDIR=$RPM_BUILD_ROOT%{_mandir}/man1 \ > BINDIR=$RPM_BUILD_ROOT%{_bindir} \ > RPMVERSION="$RPM_PACKAGE_VERSION" INSTALL='install -p' install > > You mean like this? Yes, looks good. > > Why is LD used for linking instead of gcc? > > I don't know, but does it really matter? This was upstream's decision. Looking at the files generated by LD, it could even be better to use LD and not gcc since they seem to be files that are run at the very beginning of the boot. http://togami.com/~warren/fedora/mknbi.spec http://togami.com/~warren/fedora/mknbi-1.4.4-7.fc8.src.rpm Satisfactory? A Vendor tag crept in. I spotted a license issue: md5.c has only this license information: * This implementation is copyright 1997 by M. Gutschke. This man seems to be the first etherboot maintainer, so it is likely not to be very problematic, but still. Otherwise it seems to be GPLv2+ (some file don't have any license, not a big deal, may be worth reporting upstream). Another issue is that the Source is wrong. mknbi.html should certainly be in %doc In general it is better to put perl noarch internal modules in %_datadir. And mknbi itself should certainly be in %_bindir directly. However, in the case of mknbi, it is certainly too much work and divergence from upstream to put everything at the right place. Therefore, all the rpmlint warnings are ignorable: mknbi.src:46: E: hardcoded-library-path in %{_prefix}/lib/mknbi/ mknbi.i386: E: no-binary mknbi.i386: E: only-non-binary-in-usr-lib mknbi-debuginfo.i386: E: empty-debuginfo-package http://togami.com/~warren/fedora/mknbi.spec http://togami.com/~warren/fedora/mknbi-1.4.4-8.fc8.src.rpm * Mon Oct 29 2007 Warren Togami <wtogami> 1.4.4-8 - Remove Vendor tag - mknbi.html in %%doc - Do not create (empty) debuginfo package I will contact upstream regarding license clarification, however agreed that it shouldn't block this package. The Source0 is still non existing... Also (not blockers) I think that there should be comments explaining: * the fact that optflags are not used (and the consequences on debug) * the trick that allows to build on x86_64 32 bit only * the weird placement of everything in /usr/lib/mknbi License would be better as GPLv2+ http://togami.com/~warren/fedora/mknbi.spec http://togami.com/~warren/fedora/mknbi-1.4.4-9.fc8.src.rpm * Mon Oct 29 2007 Warren Togami <wtogami> 1.4.4-9 - Fix Source0 - GPLv2+ - add some comments * doesn't really follow packaging guidelines, but it is a very special package, it is intended to create boot images, 32 bit only. No debug flags, no optflags. * source match upstream 5ddafef0a582cfb2b3cd30951662e6e2 mknbi-1.4.4.tar.gz * free software license included. Some files lack a license * follow naming guidelines Still needswork: There are perl provides that should be filtered out Provides: perl(Elf) perl(Nbi) perl(TruncFD) The source timestamp isn't kept. You can use wget -N or spectool -g. -rw-rw-r-- 1 dumas dumas 212454 aoĆ» 18 2004 mknbi-1.4.4.tar.gz -rw-rw-r-- 1 dumas dumas 212454 oct 15 04:11 ../SOURCES/mknbi-1.4.4.tar.gz http://togami.com/~warren/fedora/mknbi.spec http://togami.com/~warren/fedora/mknbi-1.4.4-10.fc8.src.rpm Satisfactory? In the future, instead of going through a cycle of extremely minor cleanups at the end like this, could you please provide patches? It would save a lot of time. Alternatively, just approve it with the condition that you be allowed to add more cleanups before the first tag & build. That would save a lot of time as well. (In reply to comment #24) > Alternatively, just approve it with the condition that you be allowed to add > more cleanups before the first tag & build. That would save a lot of time as well. > I try to say everything at once (except when there are issues one the design of the package), but in this case I discovered issues at the time I pointed them, I just missed them before. As for patches, sure I can always do some, just ask. I have one for filtering out internal modules perl requires. Created attachment 242471 [details]
filter out internal modules perl requires
With this patch (or something similar) applied, I also have * %files section is right. Although some files are not at the best place (internal noarch perl modules should be below %_datadir, mknbi should be in %_bindir or %_libexecdir) moving thing around requires some work and derive from upstream. APPROVED... unless I find something else ;-) Huh? Didn't I already add something like that? I don't see a second patch. (In reply to comment #28) > Huh? Didn't I already add something like that? > > I don't see a second patch. In comment #26, I propose a patch to filter out the Requires that are provided by the package itself, but are filtered out since they are not available as modules for real. Prior to you posting that patch, I already added something similar. Ok, it seemed to me that you only removed the autogenerated Provides. Then the corresponding autogenerated requires were still there and no package satisfied them. I know that am not clear, but one way to understand the issue is to try to install the package and if the package has the problem it won't install. Ah, nice catch. Thank you for your attention to detail. Maybe this bug should be closed? |