Bug 329291 (debootstrap-review)

rpmlint results:
debootstrap.src:51: E: hardcoded-library-path in %{_prefix}/lib/debootstrap
This is just fine -- we install no binary libraries in /usr/lib

It seems to me that using the lenny or sid version would be 
better. In my opinion, and in general, the kind of things in 
unstable match better with the innovative aspect of fedora.

In devel the build fails:
MAKEDEV -d dev std ptmx
make: MAKEDEV: Command not found

That's because MAKEDEV is in root path. You didn't built as
root, did you?

I got inspiration from Suse in the package I did here
to workaround this issue:
http://www.environnement.ens.fr/perso/dumas/fc-srpms/debian/
to workaround this issue, maybe you could have a look.

Also I install stuff in %{_datadir} to conform with the FHS.

Patrice: Thanks for your comment. Please don't confuse unstable with innovative.
If there are any things that work better in "unstable" that you miss in etch
version, feel free to tell me, I'd gladly either backport the patch or move to
unstable branch.

Also, I didn't notice that MAKEDEV does reside in /sbin as I include sbin also
in my regular path. I also built in mock, which also seems to includ /sbin in
PATH. I'll fix that do say either /sbin/MAKEDEV or /dev/MAKEDEV in next
revision. Which one do you suggest?

When it comes to FHS, I've already considered that. Here's what FHS says:

> /usr/lib includes object files, libraries, and internal binaries that are not
> intended to be executed directly by users or shell scripts. [22]

Only these two files don't comply: /usr/lib/debootstrap/arch and
/usr/lib/debootstrap/devices.tar.gz. I've choosen to leave them in /usr/lib do
avoid deviation from the upstream. If there are still strong objection against
this I will move them to /usr/share instead.

Though I found no particular bits in your package that I'd find usable for this
one, I'd be glad if you comaintained the package once it is approved.

I'll spin updated packages once I get feedback on the outstanding questions.

(In reply to comment #3)
> Patrice: Thanks for your comment. Please don't confuse unstable with innovative.
> If there are any things that work better in "unstable" that you miss in etch
> version, feel free to tell me, I'd gladly either backport the patch or move to
> unstable branch.

I don't confuse those, believe me. It is only a rule of thumb 
that works for most of the packages I have seen that are in 
debian and in fedora (in fedora devel). Otherwise said in
general stuff in debian stable is often too old for fedora
devel.

In the debootstrap I packaged there were configs for
more distros (I guess they are for ubuntu), that would be nice
to have.

> Also, I didn't notice that MAKEDEV does reside in /sbin as I include sbin also
> in my regular path. I also built in mock, which also seems to includ /sbin in
> PATH. I'll fix that do say either /sbin/MAKEDEV or /dev/MAKEDEV in next
> revision. Which one do you suggest?

/sbin/MAKEDEV
But what about the comment from Suse:
# the .deb is used to get the devices tarball as Debian
# may expect other devices as MAKEDEV generates... 

Also when I tried that, I got an error saying something along
'only root can create special devices'. But apparently this
is not the case now. Maybe because of the way you invoke it.

If your method really works, indeed it is better than my kludge.

I looked at the resulting tarballs, they are very different. But
it may be because mine is devices-std.tar.gz and yours is 
devices.tar.gz.

> When it comes to FHS, I've already considered that. Here's what FHS says:
> 
> > /usr/lib includes object files, libraries, and internal binaries that are not
> > intended to be executed directly by users or shell scripts. [22]
> 
> Only these two files don't comply: /usr/lib/debootstrap/arch and
> /usr/lib/debootstrap/devices.tar.gz. I've choosen to leave them in /usr/lib do
> avoid deviation from the upstream. If there are still strong objection against
> this I will move them to /usr/share instead.

The files in lib/debootstrap are not object files, libraries, or 
internal binaries, they are scripts functions, and script 
'configurations' and the devices tarball. All these are better 
in %_datadir. 

> Though I found no particular bits in your package that I'd find usable for this
> one, I'd be glad if you comaintained the package once it is approved.

No problem, I'd be happy to do that. I already have too much 
packages so I did not want to be the primary maintainer, but
no problem to be co-maintainer.

In fact, unless I am wrong the device-std.tar.gz is used in
debootstrap, the devices.tar.gz is used for debootstrap-udeb.

see Makefile:
install-allarch: install
        install -m 0644 devices-std.tar.gz \
                $(DSDIR)/devices.tar.gz

install-arch: install
        install -m 0755 pkgdetails $(DSDIR)/
        install -m 0644 devices.tar.gz $(DSDIR)/
        install -m 0644 debootstrap-arch $(DSDIR)/arch

and debian/rules:
        $(MAKE) install-allarch DESTDIR=$(CURDIR)/debian/debootstrap
        $(MAKE) install-arch    DESTDIR=$(CURDIR)/debian/debootstrap-udeb

So for debootstrap you should do 
make install-allarch DESTDIR=$RPM_BUILD_ROOT

It could also be possible to build a debootstrap and a 
debootstrap-udeb package, but in my opinion they should be 
named like in debian.

(In reply to comment #4)

> In the debootstrap I packaged there were configs for
> more distros (I guess they are for ubuntu), that would be nice
> to have.

Fair enough. Bumped to latest 1.0.3.

> But what about the comment from Suse:
> # the .deb is used to get the devices tarball as Debian
> # may expect other devices as MAKEDEV generates... 
> 
> Also when I tried that, I got an error saying something along
> 'only root can create special devices'. But apparently this
> is not the case now. Maybe because of the way you invoke it.

I do it in fakeroot, as you might have noticed. I chose it because it is the way
debian does it.

> The files in lib/debootstrap are not object files, libraries, or 
> internal binaries, they are scripts functions, and script 
> 'configurations' and the devices tarball. All these are better 
> in %_datadir. 

Right. Done.

(In reply to comment #5)
> In fact, unless I am wrong the device-std.tar.gz is used in
> debootstrap, the devices.tar.gz is used for debootstrap-udeb.

Right. devices-std is meant to be used when bootstraping debian installations
from within OS, rather than from installer. The collection of devices generated
by our MAKEDEV, is a bit different from what Debian uses, but after playing a
bit with it an comparing results I decided to leave it as it is. Though our one
has more devices and but lacks some, I believe that the the lacking ones weren't
instlalled purposefully, just by coincidence of being in Debian MAKEDEV's "std".
In any case, they don't deny the complete /dev from being populated after the
bootstrap -- the installed system does contain MAKEDEV in any case.

> So for debootstrap you should do 
> make install-allarch DESTDIR=$RPM_BUILD_ROOT

Also right. Fixed.

> It could also be possible to build a debootstrap and a 
> debootstrap-udeb package, but in my opinion they should be 
> named like in debian.

No need for -udeb. We are not a debian installer.

I also turned the package into noarch. I'm not completly sure whether the all of
the devices in the tarball are arch independent, as they are generated by arch
dependent MAKEDEV, but I believe it to have no impact (as ones used during
bootstrap are most likely common for different archs and after bootstrap
populating /dev with more useful content is advisable in any case.). I'll
eventually test this on ppc.

So far tested with etch/i386, sid/i386 and gutsy/i386 and confirmed to work.

Spec URL: http://people.redhat.com/lkundrak/SPECS/debootstrap.spec
SRPM URL:
http://people.redhat.com/lkundrak/mock-results/debootstrap-1.0.3-1.fc8.noarch/debootstrap-1.0.3-1.fc8.src.rpm
Mock results:
http://people.redhat.com/lkundrak/mock-results/debootstrap-1.0.3-1.fc8.noarch/

Isn't it a typo in 
debootstrap-1.0.3-devices.patch
should be dev instead of dir? That's quite strange that it
worked... Maybe the devices may be created on the fly anyway?

Also there is a path in the debootstrap script, 
    DEBOOTSTRAP_DIR=/usr/lib/debootstrap
In my spec I did:
sed -i.datadir -e
's:DEBOOTSTRAP_DIR=/usr/lib/debootstrap:DEBOOTSTRAP_DIR=%{_datadir}/%{name}/:'
debootstrap



I also used fakeroot in my tests, if I recall well but 
I must have done something wrong as indeed files are 
created and though they don't look like special files, 
they are, I even checked.

rpmlint says:
debootstrap.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 13)
debootstrap.noarch: W: spurious-executable-perm /usr/share/man/man8/debootstrap.8.gz

any progress here?

Oh, this is not finished yet -- I forgot somehow :(
I am horribly sorry and will try to address the outstanding issues today

(In reply to comment #7)
> Isn't it a typo in 
> debootstrap-1.0.3-devices.patch
> should be dev instead of dir? That's quite strange that it
> worked... Maybe the devices may be created on the fly anyway?

Right, I fixed the dir to dev.

> Also there is a path in the debootstrap script, 
>     DEBOOTSTRAP_DIR=/usr/lib/debootstrap
> In my spec I did:
> sed -i.datadir -e
> 's:DEBOOTSTRAP_DIR=/usr/lib/debootstrap:DEBOOTSTRAP_DIR=%{_datadir}/%{name}/:'
> debootstrap

Right, I have forgotten an existing /usr/lib/debootstrap in place from some
previous attempt. This might have been the reason why did the install work even
with 'dir' instead of 'dev'

> rpmlint says:
> debootstrap.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 13)
> debootstrap.noarch: W: spurious-executable-perm
/usr/share/man/man8/debootstrap.8.gz

Attempted to fix both. rpmlint is quiet now.

And soo, here are the updated packages:

SPEC: http://people.redhat.com/lkundrak/SPECS/debootstrap.spec
SRPM:
http://people.redhat.com/lkundrak/mock-results/debootstrap-1.0.3-2.fc9.noarch/debootstrap-1.0.3-2.fc9.noarch.rpm
mock: http://people.redhat.com/lkundrak/mock-results/debootstrap-1.0.3-2.fc9.noarch/

Thanks for the reminder, Parag, and thanks for the patience Patrice. I'm
horribly sorry for the delay.

You should use the latest package. The -udeb part has been
removed, it now installs in /usr/share and is much simpler.

crap :)

I will look at it

SPEC: http://people.redhat.com/lkundrak/SPECS/debootstrap.spec
SRPM:
http://people.redhat.com/lkundrak/mock-results/debootstrap-1.0.7-1.fc9.noarch/debootstrap-1.0.7-1.fc9.src.rpm
mock: http://people.redhat.com/lkundrak/mock-results/debootstrap-1.0.7-1.fc9.noarch/

Created attachment 262581 [details]
modified debootstrap-1.0.7-perms.patch to keep timestamps

* rpmlint is silent
* free software, license included
* follow packaging guidelines
* match upstream
bd446c40b90c6069178b23065d4179ca  debootstrap_1.0.7.tar.gz
* %files section right
* works

I attached a modified version of a patch to keep the timestamp
of the installed scripts. This could be perfected by a

touch -r debootstrap $RPM_BUILD_ROOT%{_sbindir}/debootstrap


Currently the rpm macros are not used because /usr/share and 
/usr/sbin are hardcoded in some places. You could overcome that by

- doing a sed s;/usr/share;%{_datadir}; on debootstrap
- doing a sed s;/usr/sbin;%{_sbindir}; on the Makefile
- passing DSDIR=$RPM_BUILD_ROOT%{_datadir}/debootstrap on the
  make install line


I think it would be better, but I won't make that a blocker.
May be done after importing.

APPROVED

I am willing to maintain EL-4 and EL-5 branches, if you don't
want to and accept me as a comaintainer for EPEL.

(In reply to comment #15)
> I attached a modified version of a patch to keep the timestamp
> of the installed scripts. This could be perfected by a
> 
> touch -r debootstrap $RPM_BUILD_ROOT%{_sbindir}/debootstrap

I don't see a good reason to keep the timestamps as this is a noarch package. I
would either keep all timestamps (adding that touch command) or not make any
effort to keep those. I choose the second option unless anyone convinces me.

> Currently the rpm macros are not used because /usr/share and 
> /usr/sbin are hardcoded in some places. You could overcome that by
> 
> - doing a sed s;/usr/share;%{_datadir}; on debootstrap
> - doing a sed s;/usr/sbin;%{_sbindir}; on the Makefile
> - passing DSDIR=$RPM_BUILD_ROOT%{_datadir}/debootstrap on the
>   make install line
> 
> I think it would be better, but I won't make that a blocker.
> May be done after importing.

I don't like the extra commands in the spec. There are no many packages that do
that and there's little chance that someone's %{_prefix} won't be /usr. I won't
mind it if you commit this to the package either :)

> APPROVED

Much thanks for the review!

> I am willing to maintain EL-4 and EL-5 branches, if you don't
> want to and accept me as a comaintainer for EPEL.

(In reply to comment #3)
> I'd be glad if you comaintained the package once it is approved.

New Package CVS Request
=======================
Package Name: debootstrap
Short Description: Bootstrap a basic Debian GNU/Linux system
Owners: lkundrak,pertusus
Branches: FC-6 F-7 F-8 EL-4 EL-5
Cvsextras Commits: yes

(In reply to comment #16)
> I don't see a good reason to keep the timestamps as this is a noarch package. I
> would either keep all timestamps (adding that touch command) or not make any
> effort to keep those. I choose the second option unless anyone convinces me.

The files timestamps give an idea on the last time they were
modified, this can be a valuable information.

> > Currently the rpm macros are not used because /usr/share and 
> > /usr/sbin are hardcoded in some places. You could overcome that by
> > 
> > - doing a sed s;/usr/share;%{_datadir}; on debootstrap
> > - doing a sed s;/usr/sbin;%{_sbindir}; on the Makefile
> > - passing DSDIR=$RPM_BUILD_ROOT%{_datadir}/debootstrap on the
> >   make install line
> > 
> > I think it would be better, but I won't make that a blocker.
> > May be done after importing.
> 
> I don't like the extra commands in the spec. There are no many packages that do
> that and there's little chance that someone's %{_prefix} won't be /usr. 

I personally always point such cases in the packages I 
review/maintain, at least when changing things is not too 
complicated, as it seems to me to be the case here.

You are right that there is a complexity versus corner case
handling. And also doing such sed substitutions requires to 
verify that they are still valid for new releases. But I find
it cleaner.

> I won't mind it if you commit this to the package either :)

Ok, I'll do the 2 changes I proposed.
 
> > I am willing to maintain EL-4 and EL-5 branches, if you don't
> > want to and accept me as a comaintainer for EPEL.
> 
> (In reply to comment #3)
> > I'd be glad if you comaintained the package once it is approved.
 
Indeed, I didn't remembered...

(In reply to comment #18)
> (In reply to comment #16)
> > I don't see a good reason to keep the timestamps as this is a noarch package. I
> > would either keep all timestamps (adding that touch command) or not make any
> > effort to keep those. I choose the second option unless anyone convinces me.
> 
> The files timestamps give an idea on the last time they were
> modified, this can be a valuable information.
I could not disagree more. They are entirely pointless.

cvs done, except for the FC-6 branch. We are no longer allowing FC-6 branches. 

Thanks, Kevin!

Patrice: Builds in all branches were initiated and succeeded. I am not going to
create the updates in bodhi now, as you wanted to make some changes.

I have rebuilt in every branches with the changes I wanted to
do.