Bug#601203: debian-cd: Recommended packages should be included recursively for apt to find

[Petter Reinholdtsen]

Package: debian-cd
Version: 3.1.3
Tags: patch
User: debian-***@lists.debian.org
UserTags: debian-edu

In Debian Edu, I have discovered that the set of packages installed
when we install using our DVD is not the same set of packages
installed when installing using the netinst CD.

I've tracked down the cause, and it is the fact that the packages we
want on the DVD do not get all their recommended packages added to the
DVD, making apt fail to find all the packages it find when using the
netinst CD. The NORECOMMENDS=0 setting in debian-cd only take effect
for the packages directly listed in the task lists. Their recommended
packages do not get their recommended packages included on the DVD.

I have written this (untested) patch to try to get debian-cd to behave
more like apt, and include recommended packages recursively when the
NORECOMMENDS=0 flag is set.

Including it here for feedback and comments. I'll test the patch and
see how it affect the Debian Edu DVD build. :)

--- debian-cd.unpatched.squeeze/tools/sort_deps (revision 2074)
+++ debian-cd.new/tools/sort_deps (working copy)
@@ -477,7 +477,7 @@

if ($add_rec) {
#TODO: Look for recommends (not yet included !!)
- add_recommends (\@dep);
+ add_recommends (\@dep, $add_rec);
# Check again but doesn't fail if one of the package cannot be
# installed, just ignore it (it will be removed from @dep)
($ok, $reasons) = check_list (\@dep, 0);
@@ -527,11 +527,12 @@

sub add_recommends {
my $list = shift;
+ my $add_rec = shift; # Do we look for recommends
my $p; # = shift;
my @copy = @{$list}; # A copy is needed since I'll modify the array

foreach $p (@copy) {
- add_missing($list, $packages{$p}{"Recommends"}, $p);
+ add_missing($list, $packages{$p}{"Recommends"}, $p, $add_rec);
}

}
@@ -554,6 +555,7 @@
my $list = shift;
my $new = shift;
my $pkgin = shift;
+ my $add_rec = shift; # Do we look for recommends
my @backup = @{$list};
my $ok = 1;

@@ -596,7 +598,7 @@
# Stop after the first package that is
# added successfully
push (@{$list}, $pkg);
- if (add_missing ($list, $packages{$pkg}{"Depends"}, $pkg)) {
+ if (add_missing ($list, $packages{$pkg}{"Depends"}, $pkg, $add_rec)) {
$or_ok = 1;
remove_entry($pkg, $list);
push @{$list}, $pkg;
@@ -622,11 +624,14 @@
next if $included{lc $_}; # Already included, don't worry
next if is_in (lc $_, $list);
push @{$list}, lc $_;
- if (not add_missing ($list, $packages{lc $_}{"Depends"}, lc $_)) {
+ if (not add_missing ($list, $packages{lc $_}{"Depends"}, lc $_, $add_rec)) {
msg(1, "couldn't add $_ ...\n");
msg(1, "$pkgin failed, couldn't satisfy dep on $_\n");
pop @{$list};
$ok = 0;
+ } elsif ($add_rec) {
+ # depends added successfully, add recommends too
+ missing ($list, $packages{lc $_}{"Recommends"}, lc $_, $add_rec);
}
remove_entry(lc $_, $list);
push @{$list}, lc $_;

Happy hacking,

[Petter Reinholdtsen]

[Petter Reinholdtsen]

Post by Petter Reinholdtsen
+ } elsif ($add_rec) {
+ # depends added successfully, add recommends too
+ missing ($list, $packages{lc $_}{"Recommends"}, lc $_, $add_rec);

The missing() call should be add_missing(). Sorry for the typo.

Happy hacking,

[Petter Reinholdtsen]

I got a working patch, but while testing and trying to get my patch
working, I discovered that the current handling of NORECOMMENDS=0 is
completely broken.

The Perl code in sort_deps to read this value look like this:

my $norecommends = $ENV{'NORECOMMENDS'} || 1;

When one ask for recommended packages to be included, the
$ENV{'NORECOMMENDS'} content resolve to "0" which is false for perl
and thus trigger the '|| 1' part, causing the opposite of the intended
effect.

This patch seem to solve this:

diff -urw debian-cd.unpatched.squeeze/tools/sort_deps debian-cd/tools/sort_deps
--- debian-cd.unpatched.squeeze/tools/sort_deps 2010-10-23 08:48:16.000000000 +02
00
+++ debian-cd/tools/sort_deps 2010-10-26 00:01:04.000000000 +0200
@@ -21,7 +21,7 @@
my $force_firmware = $ENV{'FORCE_FIRMWARE'} || 0;
my $local = $ENV{'LOCAL'} || 0;
my $complete = $ENV{'COMPLETE'} || 0;
-my $norecommends = $ENV{'NORECOMMENDS'} || 1;
+my $norecommends = (defined $ENV{'NORECOMMENDS'} ? $ENV{'NORECOMMENDS'} : 1);
my $nosuggests = $ENV{'NOSUGGESTS'} || 1;

my $apt = "$ENV{'BASEDIR'}/tools/apt-selection";

Here is the complete patch to add recommended packages, including lots
of debug output I added to try to figure out why everything I tried
failed. I'll provide a cleaner patch as soon as possible, but thought
it best to get feedback on the current changes.

diff -urw debian-cd.unpatched.squeeze/tools/sort_deps debian-cd/tools/sort_deps
--- debian-cd.unpatched.squeeze/tools/sort_deps 2010-10-23 08:48:16.000000000 +0200
+++ debian-cd/tools/sort_deps 2010-10-26 00:01:04.000000000 +0200
@@ -21,7 +21,7 @@
my $force_firmware = $ENV{'FORCE_FIRMWARE'} || 0;
my $local = $ENV{'LOCAL'} || 0;
my $complete = $ENV{'COMPLETE'} || 0;
-my $norecommends = $ENV{'NORECOMMENDS'} || 1;
+my $norecommends = (defined $ENV{'NORECOMMENDS'} ? $ENV{'NORECOMMENDS'} : 1);
my $nosuggests = $ENV{'NOSUGGESTS'} || 1;

my $apt = "$ENV{'BASEDIR'}/tools/apt-selection";
@@ -50,7 +50,7 @@
my %excluded;
my %packages;

-msg(0, "Running sort_deps to sort packages for $arch:\n");
+msg(0, "Running sort_deps to sort packages for $arch (edu):\n");
msg(1, "======================================================================
Here are the settings you've chosen for making the list:
Architecture: $arch
@@ -63,6 +63,8 @@
msg(1, yesno($nonfree)."\n");
msg(1, "Force inclusion of firmware packages: ");
msg(1, yesno($force_firmware)."\n");
+msg(1, "Include recommended packages: ");
+msg(1, yesno(!$norecommends)."\n");
msg(1, "======================================================================
");

@@ -449,7 +451,7 @@
}

# Get all dependencies (not yet included) of each package
- my (@dep) = (get_missing ($p));
+ my (@dep) = (get_missing ($p, $add_rec));

# Stop here if apt failed
if (not scalar(@dep)) {
@@ -477,7 +479,7 @@

if ($add_rec) {
#TODO: Look for recommends (not yet included !!)
- add_recommends (\@dep);
+ add_recommends (\@dep, $add_rec);
# Check again but doesn't fail if one of the package cannot be
# installed, just ignore it (it will be removed from @dep)
($ok, $reasons) = check_list (\@dep, 0);
@@ -520,30 +522,38 @@
my @copy = @{$list}; # A copy is needed since I'll modify the array

foreach $p (@copy) {
- add_missing($list, $packages{$p}{"Suggests"}, $p);
+ add_missing($list, $packages{$p}{"Suggests"}, $p, 0);
}

}

sub add_recommends {
my $list = shift;
+ my $add_rec = shift; # Do we look for recommends
my $p; # = shift;
my @copy = @{$list}; # A copy is needed since I'll modify the array

foreach $p (@copy) {
- add_missing($list, $packages{$p}{"Recommends"}, $p);
+ add_missing($list, $packages{$p}{"Recommends"}, $p, $add_rec);
}

}

sub get_missing {
my $p = shift;
+ my $add_rec = shift; # Do we look for recommends
my @list = ();

- if (not add_missing (\@list, $packages{$p}{"Depends"}, $p)) {
+ msg(0, "Looking for Depends for $p (add-rec = $add_rec)\n");
+ if (not add_missing (\@list, $packages{$p}{"Depends"}, $p, $add_rec)) {
return ();
}

+ if ($add_rec) {
+ msg(0, "Looking for Recommends for $p\n");
+ add_missing (\@list, $packages{$p}{"Recommends"}, $p, $add_rec);
+ }
+
remove_entry($p, \@list);
push @list, $p;
return (@list);
@@ -554,6 +564,7 @@
my $list = shift;
my $new = shift;
my $pkgin = shift;
+ my $add_rec = shift; # Do we look for recommends
my @backup = @{$list};
my $ok = 1;

@@ -596,7 +607,7 @@
# Stop after the first package that is
# added successfully
push (@{$list}, $pkg);
- if (add_missing ($list, $packages{$pkg}{"Depends"}, $pkg)) {
+ if (add_missing ($list, $packages{$pkg}{"Depends"}, $pkg, $add_rec)) {
$or_ok = 1;
remove_entry($pkg, $list);
push @{$list}, $pkg;
@@ -622,11 +633,16 @@
next if $included{lc $_}; # Already included, don't worry
next if is_in (lc $_, $list);
push @{$list}, lc $_;
- if (not add_missing ($list, $packages{lc $_}{"Depends"}, lc $_)) {
+ if (not add_missing ($list, $packages{lc $_}{"Depends"}, lc $_, $add_rec)) {
msg(1, "couldn't add $_ ...\n");
msg(1, "$pkgin failed, couldn't satisfy dep on $_\n");
pop @{$list};
$ok = 0;
+ } elsif ($add_rec) {
+ my $reclist = $packages{lc $_}{"Recommends"};
+ msg(0, "trying to add recommends $reclist ...\n");
+ # depends added successfully, add recommends too
+ add_missing ($list, $reclist, lc $_, $add_rec);
}
remove_entry(lc $_, $list);
push @{$list}, lc $_;


Happy hacking,

[Holger Levsen]

Hi Steve,

Wolfgang tested a patch for "#601203: tested patch for adding support for
recursivly including recommends" and asked me for review and pushing it
into debian-cd.git.

As I know how busy you are incl. with the shim-signed stuff, I've
decided to push it to a h01ger/601203 branch and not master :)

This patch (or rather these two small patches...) have been proven to work
using the simple-cdd wrapper for debian-cd to build an ISO image like this:
DISKTYPE=BD NORECOMMENDS=0 build-simple-cdd --dist buster --profiles xfce

where xfce refers to a profile file ./profiles/xfce.packages which has a single
entry, 'task-xfce-desktop'. This causes all recommends to be on the
generated ISO.

If called with NORECOMMENDS=1 (or NORECOMMENDS unset) it works as
expected and doesnt include any recommends on the ISO (just the depends
are there then.).

(A second test has made with the same results using the
education-ltsp-server metapackage instead of xfce.)

We (Debian Edu) really really really would like to see this merged, as else
the maintenance of the Edu BD image will be a PITA as the education-tasks-*
metapackages heavily use recommends (due to the design of the blends-dev
framework).

For convinience I've also attached the patches to this mail.

[Arnaud Rebillout]

Hi all,

I updated the patches so that they apply on current debian-cd, they are
attached to this email. Only the patch 0001 needed to be reworked, the
patch 0002 is left unchanged.

However it seems that the patch doesn't really solve the issue. For my
case, it seems that there's around 100 packages that are missed by
sort_deps. After applying the patch, maybe ~ 90 packages are still
missing. Not much improvement.

[Philip Hands]

Hi

Skimming through this thread as a result of your mail, I noticed the
earlier patch replacing the || 1 with defined ... ? ... and was going to
point out that since 2007 perl's had the // operator for this sort of
thing, which should mean that this would do the trick these days:

my $norecommends = $ENV{'NORECOMMENDS'} // 1;

-my $norecommends = read_env('NORECOMMENDS', 1);
+my $norecommends = (defined $ENV{'NORECOMMENDS'} ? $ENV{'NORECOMMENDS'} : 1);

which replaces the use of read_env() with the earlier fix, which strikes
me as a backwards step, given that read_env is doing exactly what's
needed here:

=-=-=-
sub read_env {
my $env_var = shift;
my $default = shift;

if (exists($ENV{$env_var})) {
return $ENV{$env_var};
}
# else
return $default;
}
=-=-=-

Cheers, Phil.

[Raphael Hertzog]

Control: tags -1 + pending

Hello,

I fixed this issue in git now:
https://salsa.debian.org/images-team/debian-cd/-/commit/8513b237afbdb59a4a59d1bc707d37f0aa9138f7

This was on top of a first cleanup where I turned $add_rec and $add_sug
into global variables:
https://salsa.debian.org/images-team/debian-cd/-/commit/c9489cd926f9b09b1359c17cc651a7a664e537a7

I think it would be great if we could make a new debian-cd upload as we
have some important fixes in git by now.

Steve, do you want to review my latest changes and do an upload?

Let me know if you need help for the upload.

Cheers,

[Debian Bug Tracking System]

Post by Raphael Hertzog
tags -1 + pending

Bug #601203 [debian-cd] debian-cd: Recommended packages should be included recursively for apt to find
Added tag(s) pending.

[Debian Bug Tracking System]

Your message dated Sun, 15 Jan 2023 12:35:05 +0000
with message-id <E1pH2E5-009iaH-***@fasolo.debian.org>
and subject line Bug#601203: fixed in debian-cd 3.1.36
has caused the Debian Bug report #601203,
regarding debian-cd: Recommended packages should be included recursively for apt to find
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact ***@bugs.debian.org
immediately.)