Re: [PATCH] New Dpkg::Deps module to replace parsedep() and showdep()
Hi,
On Mon, 2007-10-15 at 15:55:13 +0200, Raphael Hertzog wrote:
> Please review and run it. I think it can be merged soon, just tell me to
> go ahead when you're confident. It would be nice to include it in the
> upcoming release as it automatically removes duplicate dependencies
> potentially introduced by dpkg-shlibdeps (and thus renders my wishlist bug
> #443973 useless).
Cool, thanks for working on this!
I've not tested it yet, just reviewed. I'll try to do the former tomorrow.
> I think that adding some POD documentation to the module would help. Feel
> free to jump in.
I was thinking that we could do that for stuff that we are satisfied being
part of the public API. And if it's not documented then it should not be
used externally, and should be expected to change.
> There's also two "die" functions that should probably by
> replaced some Dpkg::ErrorHandling functions. But as they are errors for
> programmers only (i.e. not the kind of error that an end-user will see), I
> left them as-is for the moment.
>
> (I have the feeling we already require translation of too many strings
> that users won't never see)
I agree with Frank on this.
Just found some minor issues:
> >From 68e0d3b4599cc855316a50f13f8f4d697c3f2a04 Mon Sep 17 00:00:00 2001
> From: Raphael Hertzog <hertzog@debian.org>
> Date: Sun, 14 Oct 2007 23:27:13 +0200
> Subject: [PATCH] Add new module Dpkg::Deps to handle dependencies and some corresponding tests
> diff --git a/scripts/Dpkg/Deps.pm b/scripts/Dpkg/Deps.pm
> +use Dpkg::Gettext;
> +textdomain("dpkg-dev");
No need to call textdomain inside the modules (those can only be used
in dpkg-dev programs) and the function is called already on the programs
anyway.
> >From be01cb1c9ebdb325c3818cfe83dff5380e8075bc Mon Sep 17 00:00:00 2001
> From: Raphael Hertzog <hertzog@debian.org>
> Date: Mon, 15 Oct 2007 12:19:59 +0200
> Subject: [PATCH] dpkg-gencontrol: use the new Dpkg::Deps module to rewrite the dependencies
> diff --git a/scripts/dpkg-gencontrol.pl b/scripts/dpkg-gencontrol.pl
> @@ -14,10 +14,13 @@ use Dpkg::Arch qw(get_host_arch debarch_eq debarch_is);
> push(@INC,$dpkglibdir);
> require 'controllib.pl';
>
> +use Dpkg::Deps qw(@pkg_dep_fields %dep_field_type);
> +
> our %substvar;
> our (%f, %fi);
> our %p2i;
> our @pkg_dep_fields;
> +our %dep_field_type;
Those two can be removed instead, as they have been explicitely imported
now. Also please put the 'use' with the others, I placed the our list
there, close to controllib, as a reminder of what we are using from it.
> @@ -226,15 +229,43 @@ init_substvar_arch();
> # Process dependency fields in a second pass, now that substvars have been
> # initialized.
>
> -for $_ (keys %fi) {
> - my $v = $fi{$_};
> +my $facts = Dpkg::Deps::KnownFacts->new();
> +$facts->add_installed_package($f{'Package'}, $f{'Version'});
> +if (exists $fi{"C$myindex Provides"}) {
> + my $provides = Dpkg::Deps::parse(substvars($fi{"C$myindex Provides"}),
> + reduce_arch => 1, union => 1);
Do not use tabs for alignment (only for indenting).
> + if (defined $provides) {
> + foreach my $subdep ($provides->get_deps()) {
> + if ($subdep->isa('Dpkg::Deps::Simple')) {
> + $facts->add_provided_package($subdep->{package},
> + $subdep->{relation}, $subdep->{version},
> + $f{'Package'});
Ditto.
> +my (@seen_deps);
> +foreach my $field (@pkg_dep_fields) {
> + my $key = "C$myindex $field";
> + if (exists $fi{$key}) {
> + my $dep;
> + if ($dep_field_type{$field} eq 'normal') {
> + $dep = Dpkg::Deps::parse(substvars($fi{$key}), use_arch => 1,
> + reduce_arch => 1);
Ditto.
> + &error(sprintf(_g("error occurred while parsing %s"), $_)) unless defined $dep;
The error function calls will have to be fixed for the new format string
support.
> + $dep->simplify_deps($facts, @seen_deps);
> + # Remember normal deps to simplify even further weaker deps
> + push @seen_deps, $dep if $dep_field_type{$field} eq 'normal';
> + } else {
> + $dep = Dpkg::Deps::parse(substvars($fi{$key}), use_arch => 1,
> + reduce_arch => 1, union => 1);
Alignment.
> >From f04e7c9e5cb680a3ec1433f6fcd30d772d91b2b1 Mon Sep 17 00:00:00 2001
> From: Raphael Hertzog <hertzog@debian.org>
> Date: Mon, 15 Oct 2007 14:27:27 +0200
> Subject: [PATCH] dpkg-source: use the new Dpkg::Deps module instead of controllib's parsedep
> diff --git a/scripts/dpkg-source.pl b/scripts/dpkg-source.pl
> @@ -315,9 +316,21 @@ if ($opmode eq 'build') {
> }
> elsif (m/^Uploaders$/i) { ($f{$_}= $v) =~ s/[\r\n]//g; }
> elsif (m/^Build-(Depends|Conflicts)(-Indep)?$/i) {
> - my $dep = parsedep($v, 1);
> + my $dep;
> + # XXX: Should use %dep_field_type to decide if we parse
> + # as union or not but since case-insensitive matching is
> + # used, I can't rely on $_ to have the very same
> + # capitalization...
> + if (lc($1) eq "depends") {
> + $dep = Dpkg::Deps::parse($v);
> + } else {
> + $dep = Dpkg::Deps::parse($v, union => 1);
> + }
You can use capit() from controllib.pl. Although it might be better to
normalize all fields at parse/input time instead of all over the place
and switch to case-sensitive matching, but we can fix that later globally.
> >From 43b32199bad06a38d384fb9bf619dcbdcf1d5b39 Mon Sep 17 00:00:00 2001
> From: Raphael Hertzog <hertzog@debian.org>
> Date: Mon, 15 Oct 2007 14:59:31 +0200
> Subject: [PATCH] dpkg-checkbuilddeps: modify to use the new Dpkg::Deps module
> diff --git a/scripts/dpkg-checkbuilddeps.pl b/scripts/dpkg-checkbuilddeps.pl
> @@ -47,67 +48,71 @@ my $controlfile = shift || "debian/control";
> my $dep_regex=qr/[ \t]*(([^\n]+|\n[ \t])*)\s/; # allow multi-line
> if (defined($fi{"C Build-Depends"})) {
> push @unmet, build_depends('Build-Depends',
> - parsedep($fi{"C Build-Depends"}, 1, 1),
> - @status);
> + Dpkg::Deps::parse($fi{"C Build-Depends"},
> + reduce_arch => 1), $facts);
> }
> if (defined($fi{"C Build-Conflicts"})) {
> push @conflicts, build_conflicts('Build-Conflicts',
> - parsedep($fi{"C Build-Conflicts"}, 1, 1),
> - @status);
> + Dpkg::Deps::parse($fi{"C Build-Conflicts"},
> + reduce_arch => 1, union => 1), $facts);
> }
> if (! $binary_only && defined($fi{"C Build-Depends-Indep"})) {
> push @unmet, build_depends('Build-Depends-Indep',
> - parsedep($fi{"C Build-Depends-Indep"}, 1, 1),
> - @status);
> + Dpkg::Deps::parse($fi{"C Build-Depends-Indep"},
> + reduce_arch => 1), $facts);
> }
> if (! $binary_only && defined($fi{"C Build-Conflicts-Indep"})) {
> push @conflicts, build_conflicts('Build-Conflicts-Indep',
> - parsedep($fi{"C Build-Conflicts-Indep"}, 1, 1),
> - @status);
> + Dpkg::Deps::parse($fi{"C Build-Conflicts-Indep"},
> + reduce_arch => 1, union => 1), $facts);
> }
Alignments.
> +# Silly little status file parser that returns a Dpkg::Deps::KnownFacts
> sub parse_status {
> if (/^Provides: (.*)$/m) {
> - foreach (split(/,\s*/, $1)) {
> - push @{$providers{$_}}, $package;
> + my $provides = Dpkg::Deps::parse($1,
> + reduce_arch => 1, union => 1);
> + next if not defined $provides;
> + foreach (grep { $_->isa('Dpkg::Deps::Simple') }
> + $provides->get_deps())
> + {
> + $facts->add_provided_package($_->{package},
> + $_->{relation}, $_->{version},
> + $package);
Alignments.
> >From fbb5738a9a6cf9f30e2b84bf4479ee12e5768d39 Mon Sep 17 00:00:00 2001
> From: Raphael Hertzog <hertzog@debian.org>
> Date: Mon, 15 Oct 2007 15:38:19 +0200
> Subject: [PATCH] Update changelog entries concerning the integration of Dpkg::Deps.
> diff --git a/ChangeLog b/ChangeLog
> @@ -7,6 +7,10 @@
> exports ".." as $pkgdatadir during tests).
> * scripts/dpkg-shlibdeps.pl: bugfix, avoid unwanted modification
> of @pkg_shlibs by my_find_library.
> + * scripts/dpkg-{checkbuilddeps.pl,gencontrol.pl,source.pl}:
> + adapted to use the new Dpkg::Deps module. dpkg-gencontrol gains
> + new features such as automatic simplification of dependencies.
> + * scripts/controllib.pl: remove unused parsedep() and showdep().
Please do not use globs in changelogs, as it makes using grep more
difficult.
regards,
guillem
Reply to: