Bug 331631 - Review Request: mknbi - Utility for creating network bootable images
Review Request: mknbi - Utility for creating network bootable images
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Extras Quality Assurance
:
Depends On:
Blocks: K12LTSP
  Show dependency treegraph
 
Reported: 2007-10-14 22:28 EDT by Eric Harrison
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-11-14 11:37:04 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
pertusus: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)
filter out internal modules perl requires (582 bytes, patch)
2007-10-29 18:16 EDT, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Eric Harrison 2007-10-14 22:28:14 EDT
Spec URL: http://k12linux.mesd.k12.or.us/K12LTSP/source/mknbi/mknbi.spec
SRPM URL: http://k12linux.mesd.k12.or.us/K12LTSP/source/mknbi/mknbi-1.4.4-3.fc8.src.rpm
Description: Utility for creating network bootable images

Slightly funky package in that it will only build on i386, but runs on all arches (perl script executables). There was an old Fedora Extras 4 package for mknbi & I also took a look at the syslinux package which has similar build/install oddities.
Comment 1 Jason Tibbitts 2007-10-14 23:05:23 EDT
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.
Comment 2 Eric Harrison 2007-10-14 23:27:56 EDT
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...
Comment 3 Eric Harrison 2007-10-15 00:17:35 EDT
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


Comment 4 manuel wolfshant 2007-10-15 04:49:49 EDT
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.
Comment 5 Bill Nottingham 2007-10-15 14:23:44 EDT
What does this do that network boot support in mkinitrd doesn't?
Comment 6 Chris Ricker 2007-10-15 14:29:56 EDT
It makes images that can be booted by etherboot or similar - rom boot loaders
embedded on the NIC

Comment 7 Eric Harrison 2007-10-15 14:43:39 EDT
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).
Comment 8 Warren Togami 2007-10-19 15:52:02 EDT
http://togami.com/~warren/fedora/mknbi-1.4.4-5.fc8.src.rpm

* Fri Oct 19 2007 Warren Togami <wtogami@redhat.com> 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?
Comment 9 Warren Togami 2007-10-19 16:10:09 EDT
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?
Comment 10 Patrice Dumas 2007-10-19 20:45:51 EDT
(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)
Comment 11 Warren Togami 2007-10-19 22:55:11 EDT
> 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.
Comment 12 Warren Togami 2007-10-25 16:53:11 EDT
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.
Comment 13 Patrice Dumas 2007-10-25 17:08:15 EDT
perl should be in BuildRequires now.

Why is LD used for linking instead of gcc?
Comment 14 Patrice Dumas 2007-10-25 17:11:28 EDT
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}
Comment 15 Warren Togami 2007-10-28 22:21:09 EDT
%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.
Comment 16 Patrice Dumas 2007-10-29 06:23:55 EDT
(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.
Comment 18 Patrice Dumas 2007-10-29 11:56:23 EDT
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
Comment 19 Warren Togami 2007-10-29 12:41:40 EDT
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@redhat.com> 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.
Comment 20 Patrice Dumas 2007-10-29 14:08:58 EDT
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+
Comment 21 Warren Togami 2007-10-29 14:48:22 EDT
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@redhat.com> 1.4.4-9
- Fix Source0
- GPLv2+
- add some comments
Comment 22 Patrice Dumas 2007-10-29 17:07:14 EDT
* 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
Comment 23 Warren Togami 2007-10-29 17:58:23 EDT
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.
Comment 24 Warren Togami 2007-10-29 17:58:59 EDT
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.
Comment 25 Patrice Dumas 2007-10-29 18:15:20 EDT
(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.
Comment 26 Patrice Dumas 2007-10-29 18:16:06 EDT
Created attachment 242471 [details]
filter out internal modules perl requires
Comment 27 Patrice Dumas 2007-10-29 18:19:44 EDT
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 ;-)
Comment 28 Warren Togami 2007-10-29 20:18:06 EDT
Huh?  Didn't I already add something like that?

I don't see a second patch.
Comment 29 Patrice Dumas 2007-10-30 16:34:16 EDT
(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.
Comment 30 Warren Togami 2007-10-30 17:17:07 EDT
Prior to you posting that patch, I already added something similar.
Comment 31 Patrice Dumas 2007-10-30 17:27:20 EDT
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.
Comment 32 Warren Togami 2007-10-30 17:57:53 EDT
Ah, nice catch.  Thank you for your attention to detail.
Comment 33 Patrice Dumas 2007-11-14 03:34:29 EST
Maybe this bug should be closed?

Note You need to log in before you can comment on or make changes to this bug.