add mirror discovery to pkg_add

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

add mirror discovery to pkg_add

Marc Espie-2
Most of the code was already there.

This would allow pkg_add to auto-configure a mirror, for the case where
PKG_PATH was not specified and where pkg.conf does not exist.

It only triggers when a location ends up empty and when run in interactive
mode, e.g., it shouldn't interfere with local lookups.

Good idea, or awful ?

Index: OpenBSD/PackageLocator.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PackageLocator.pm,v
retrieving revision 1.105
diff -u -p -r1.105 PackageLocator.pm
--- OpenBSD/PackageLocator.pm 30 Jan 2016 11:29:29 -0000 1.105
+++ OpenBSD/PackageLocator.pm 22 Jun 2016 13:51:40 -0000
@@ -24,6 +24,7 @@ use OpenBSD::PackageRepositoryList;
 use OpenBSD::PackageRepository;
 
 my $default_path;
+my $is_configured;
 
 sub build_default_path
 {
@@ -37,17 +38,91 @@ sub build_default_path
  while (my $o = OpenBSD::PackageRepository->parse(\$v, $state)) {
  $default_path->add($o);
  }
+ $is_configured = 1;
  return;
  }
  $default_path->add(OpenBSD::PackageRepository->new("./", $state)->can_be_empty);
- return if $state->defines('NOINSTALLPATH');
+ if ($state->defines('NOINSTALLPATH')) {
+ $is_configured = 1;
+ return;
+ }
 
  return unless defined $state->config->value('installpath');
+ $is_configured = 1;
  for my $i ($state->config->value("installpath")) {
  $default_path->add(OpenBSD::PackageRepository->new($i, $state));
  }
 }
 
+sub discover_mirror
+{
+ my ($self, $state) = @_;
+
+ # can't ask the user -> no mirror
+ return undef unless $state->is_interactive;
+
+
+ require OpenBSD::PackageRepository;
+ my $fake = OpenBSD::PackageRepository->new("http://129.128.5.191/cgi-bin/", $state);
+ # XXX
+ bless $fake, "OpenBSD::PackageRepository::Cgi";
+ my $l = $fake->list;
+ my @m = @$l;
+ my %h;
+ for my $d (@m) {
+ my $e = $d;
+ $d =~ s,^http://(.*?)(/.*?)?\s+(.*)$,$1\t$3,;
+ $e =~ s/\s+.*$//;
+ $h{$d} = $e;
+ }
+ $m[0] = "<None>";
+ my $i = $state->ask_list("No mirror configured, choose one", @m);
+ if ($i eq "<None>") {
+ return undef;
+ }
+ return $h{$i};
+}
+
+sub convert_to_packages
+{
+ my ($self, $url) = @_;
+ # mirror was "designed" for base releases.
+ # convert into short installpath version
+ $url =~ s,^http://(.*)/pub/OpenBSD$,$1, or
+    $url =~ s,$,/%c/packages/%a,;
+ return $url;
+}
+
+sub last_chance
+{
+ if ($is_configured) {
+ return [];
+ }
+ $is_configured = 1;
+ my ($self, @search) = @_;
+ my $state = pop @search;
+
+ my $url = $self->discover_mirror($state);
+ if (!defined $url) {
+ return [];
+ }
+
+ $url = $self->convert_to_packages($url);
+
+ # try setting it "permanently"
+ if (open(my $f, ">>", OpenBSD::Paths->pkgconf)) {
+ print $f "installpath += $url\n";
+ close $f;
+ } else {
+ $state->errsay("Couldn't write to #1", OpenBSD::Paths->pkgconf);
+ }
+
+ # use it for the current round anyway
+ $default_path->add(OpenBSD::PackageRepository->new($url, $state));
+
+ return $self->match_locations(@search, $state);
+}
+
 sub default_path
 {
  if (!defined $default_path) {
@@ -107,4 +182,27 @@ sub match_locations
  return $self->default_path($state)->match_locations(@search);
 }
 
+package OpenBSD::PackageRepository::Cgi;
+our @ISA = qw(OpenBSD::PackageRepository::HTTP);
+
+# we know how to get a list, we just need to override the specific url
+# and parser
+sub get_http_list
+{
+ my ($self, $error) = @_;
+
+ require OpenBSD::Paths;
+ my $fullname = $self->url."ftplist.cgi?path=".OpenBSD::Paths->os_directory."/".OpenBSD::Paths->machine_architecture;
+ my $l = [];
+ my $fh = $self->open_read_ftp(OpenBSD::Paths->ftp." -o - $fullname",
+    $error) or return;
+ while(<$fh>) {
+ chomp;
+ if (m/^http:\/\//) {
+ push(@$l, $_);
+ }
+ }
+ $self->close_read_ftp($fh);
+ return $l;
+}
 1;
Index: OpenBSD/PackageRepositoryList.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PackageRepositoryList.pm,v
retrieving revision 1.30
diff -u -p -r1.30 PackageRepositoryList.pm
--- OpenBSD/PackageRepositoryList.pm 9 Jul 2015 12:57:55 -0000 1.30
+++ OpenBSD/PackageRepositoryList.pm 22 Jun 2016 13:51:40 -0000
@@ -86,7 +86,7 @@ sub match_locations
  return $l;
  }
  }
- return [];
+ return $self->{state}->repo->last_chance(@search);
 }
 
 1;
Index: OpenBSD/State.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/State.pm,v
retrieving revision 1.34
diff -u -p -r1.34 State.pm
--- OpenBSD/State.pm 6 Apr 2015 11:07:24 -0000 1.34
+++ OpenBSD/State.pm 22 Jun 2016 13:51:40 -0000
@@ -137,6 +137,14 @@ sub match_locations
  return OpenBSD::PackageLocator->match_locations(@_, $self->{state});
 }
 
+sub last_chance
+{
+ my $self = shift;
+ require OpenBSD::PackageLocator;
+
+ return OpenBSD::PackageLocator->last_chance(@_, $self->{state});
+}
+
 sub grabPlist
 {
  my ($self, $url, $code) = @_;
@@ -340,6 +348,11 @@ sub handle_options
  }
  local $Exporter::ExportLevel = $state->{export_level};
  import OpenBSD::State;
+}
+
+sub is_interactive
+{
+ return 0;
 }
 
 sub defines

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Marc Espie-2
Here's an expanded version of the patch.
So far, ask_list was happy with prompting, but the mirror list is slightly
large, so being able to pipe thru more comes in handy.

This means a bit of refactor: we've got state, so we can get the height
from a progressmeter (or the stub), and it's reasonably easy to tweak
state display to be able to use arbitrary fh...

This shows another logic limitation of the current code, namely that the
size detection of the display is linked to the progressmeter options, whereas
it should more or less always happen when we detect we got a tty connected,
since the width/height information is uncorrelated to whether or not we
need/want a progressmeter.

To be continued...

Index: OpenBSD/AddCreateDelete.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/AddCreateDelete.pm,v
retrieving revision 1.37
diff -u -p -r1.37 AddCreateDelete.pm
--- OpenBSD/AddCreateDelete.pm 15 Jun 2016 15:40:13 -0000 1.37
+++ OpenBSD/AddCreateDelete.pm 22 Jun 2016 15:55:29 -0000
@@ -43,6 +43,11 @@ sub progress
  return $self->{progressmeter};
 }
 
+sub height
+{
+ my $self = shift;
+ return $self->{progressmeter}->height;
+}
 sub not
 {
  my $self = shift;
Index: OpenBSD/Interactive.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/Interactive.pm,v
retrieving revision 1.20
diff -u -p -r1.20 Interactive.pm
--- OpenBSD/Interactive.pm 30 Jan 2015 11:42:55 -0000 1.20
+++ OpenBSD/Interactive.pm 22 Jun 2016 15:55:29 -0000
@@ -35,13 +35,23 @@ sub ask_list
  if ($self->{always}) {
  return $values[0];
  }
+ my ($fh, $pid);
+ if ($self->{state}->height <= @values + 1) {
+ $pid = open($fh, "|-", "more", "-c");
+ }
 
- $self->{state}->errsay('#1', $prompt);
+ $fh //= \*STDERR;
+
+ $self->{state}->fhsay($fh, '#1', $prompt);
  my $i = 0;
  for my $v (@values) {
- $self->{state}->errsay("#1\t#2: #3",
+ $self->{state}->fhsay($fh, "#1\t#2: #3",
     $i == 0 ? "a" : "", $i, $v);
  $i++;
+ }
+ if (defined $pid) {
+ close($fh);
+ waitpid $pid, 0;
  }
 LOOP:
  $self->{state}->errprint("Your choice: ");
Index: OpenBSD/PackageLocator.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PackageLocator.pm,v
retrieving revision 1.105
diff -u -p -r1.105 PackageLocator.pm
--- OpenBSD/PackageLocator.pm 30 Jan 2016 11:29:29 -0000 1.105
+++ OpenBSD/PackageLocator.pm 22 Jun 2016 15:55:29 -0000
@@ -24,6 +24,7 @@ use OpenBSD::PackageRepositoryList;
 use OpenBSD::PackageRepository;
 
 my $default_path;
+my $is_configured;
 
 sub build_default_path
 {
@@ -37,17 +38,93 @@ sub build_default_path
  while (my $o = OpenBSD::PackageRepository->parse(\$v, $state)) {
  $default_path->add($o);
  }
+ $is_configured = 1;
  return;
  }
  $default_path->add(OpenBSD::PackageRepository->new("./", $state)->can_be_empty);
- return if $state->defines('NOINSTALLPATH');
+ if ($state->defines('NOINSTALLPATH')) {
+ $is_configured = 1;
+ return;
+ }
 
  return unless defined $state->config->value('installpath');
+ $is_configured = 1;
  for my $i ($state->config->value("installpath")) {
  $default_path->add(OpenBSD::PackageRepository->new($i, $state));
  }
 }
 
+sub discover_mirror
+{
+ my ($self, $state) = @_;
+
+ # can't ask the user -> no mirror
+ return undef unless $state->is_interactive;
+
+
+ require OpenBSD::PackageRepository;
+ # ftp.openbsd.org == 129.128.5.191 and will remain at
+ # that address for the foreseeable future.
+ my $fake = OpenBSD::PackageRepository->new("http://129.128.5.191/cgi-bin/", $state);
+ # XXX
+ bless $fake, "OpenBSD::PackageRepository::Cgi";
+ my $l = $fake->list;
+ my @m = @$l;
+ my %h;
+ for my $d (@m) {
+ my $e = $d;
+ $d =~ s,^http://(.*?)(/.*?)?\s+(.*)$,$1\t$3,;
+ $e =~ s/\s+.*$//;
+ $h{$d} = $e;
+ }
+ $m[0] = "<None>";
+ my $i = $state->ask_list("No mirror configured, choose one", @m);
+ if ($i eq "<None>") {
+ return undef;
+ }
+ return $h{$i};
+}
+
+sub convert_to_packages
+{
+ my ($self, $url) = @_;
+ # mirror was "designed" for base releases.
+ # convert into short installpath version
+ $url =~ s,^http://(.*)/pub/OpenBSD$,$1, or
+    $url =~ s,$,/%c/packages/%a,;
+ return $url;
+}
+
+sub last_chance
+{
+ if ($is_configured) {
+ return [];
+ }
+ $is_configured = 1;
+ my ($self, @search) = @_;
+ my $state = pop @search;
+
+ my $url = $self->discover_mirror($state);
+ if (!defined $url) {
+ return [];
+ }
+
+ $url = $self->convert_to_packages($url);
+
+ # try setting it "permanently"
+ if (open(my $f, ">>", OpenBSD::Paths->pkgconf)) {
+ print $f "installpath += $url\n";
+ close $f;
+ } else {
+ $state->errsay("Couldn't write to #1", OpenBSD::Paths->pkgconf);
+ }
+
+ # use it for the current round anyway
+ $default_path->add(OpenBSD::PackageRepository->new($url, $state));
+
+ return $self->match_locations(@search, $state);
+}
+
 sub default_path
 {
  if (!defined $default_path) {
@@ -107,4 +184,27 @@ sub match_locations
  return $self->default_path($state)->match_locations(@search);
 }
 
+package OpenBSD::PackageRepository::Cgi;
+our @ISA = qw(OpenBSD::PackageRepository::HTTP);
+
+# we know how to get a list, we just need to override the specific url
+# and parser
+sub get_http_list
+{
+ my ($self, $error) = @_;
+
+ require OpenBSD::Paths;
+ my $fullname = $self->url."ftplist.cgi?path=".OpenBSD::Paths->os_directory."/".OpenBSD::Paths->machine_architecture;
+ my $l = [];
+ my $fh = $self->open_read_ftp(OpenBSD::Paths->ftp." -o - $fullname",
+    $error) or return;
+ while(<$fh>) {
+ chomp;
+ if (m/^http:\/\//) {
+ push(@$l, $_);
+ }
+ }
+ $self->close_read_ftp($fh);
+ return $l;
+}
 1;
Index: OpenBSD/PackageRepositoryList.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/PackageRepositoryList.pm,v
retrieving revision 1.30
diff -u -p -r1.30 PackageRepositoryList.pm
--- OpenBSD/PackageRepositoryList.pm 9 Jul 2015 12:57:55 -0000 1.30
+++ OpenBSD/PackageRepositoryList.pm 22 Jun 2016 15:55:29 -0000
@@ -86,7 +86,7 @@ sub match_locations
  return $l;
  }
  }
- return [];
+ return $self->{state}->repo->last_chance(@search);
 }
 
 1;
Index: OpenBSD/ProgressMeter.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/ProgressMeter.pm,v
retrieving revision 1.48
diff -u -p -r1.48 ProgressMeter.pm
--- OpenBSD/ProgressMeter.pm 15 Jun 2016 15:31:09 -0000 1.48
+++ OpenBSD/ProgressMeter.pm 22 Jun 2016 15:55:30 -0000
@@ -103,6 +103,10 @@ sub ntogo
  return "";
 }
 
+sub height
+{
+ return 24;
+}
 sub visit_with_size
 {
  my ($progress, $plist, $method, @r) = @_;
Index: OpenBSD/State.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/State.pm,v
retrieving revision 1.34
diff -u -p -r1.34 State.pm
--- OpenBSD/State.pm 6 Apr 2015 11:07:24 -0000 1.34
+++ OpenBSD/State.pm 22 Jun 2016 15:55:30 -0000
@@ -137,6 +137,14 @@ sub match_locations
  return OpenBSD::PackageLocator->match_locations(@_, $self->{state});
 }
 
+sub last_chance
+{
+ my $self = shift;
+ require OpenBSD::PackageLocator;
+
+ return OpenBSD::PackageLocator->last_chance(@_, $self->{state});
+}
+
 sub grabPlist
 {
  my ($self, $url, $code) = @_;
@@ -257,50 +265,59 @@ sub fatal
  $self->_fatal($self->f(@_));
 }
 
-sub _print
+sub _fhprint
 {
  my $self = shift;
+ my $fh = shift;
  $self->sync_display;
- print @_;
+ print $fh @_;
+}
+
+sub _print
+{
+ my $self = shift;
+ $self->_fhprint(\*STDOUT, @_);
 }
 
 sub _errprint
 {
  my $self = shift;
- $self->sync_display;
- print STDERR @_;
+ $self->_fhprint(\*STDERR, @_);
 }
 
 sub print
 {
  my $self = shift;
- $self->_print($self->f(@_));
+ $self->_fhprint(\*STDOUT, $self->f(@_));
 }
 
-sub say
+sub errprint
 {
  my $self = shift;
+ $self->_fhprint(\*STDERR, $self->f(@_));
+}
+
+sub fhsay
+{
+ my $self = shift;
+ my $fh = shift;
  if (@_ == 0) {
- $self->_print("\n");
+ $self->_fhprint($fh, "\n");
  } else {
- $self->_print($self->f(@_), "\n");
+ $self->_fhprint($fh, $self->f(@_), "\n");
  }
 }
 
-sub errprint
+sub say
 {
  my $self = shift;
- $self->_errprint($self->f(@_));
+ $self->fhsay(\*STDOUT, @_);
 }
 
 sub errsay
 {
  my $self = shift;
- if (@_ == 0) {
- $self->_errprint("\n");
- } else {
- $self->_errprint($self->f(@_), "\n");
- }
+ $self->fhsay(\*STDERR, @_);
 }
 
 sub do_options
@@ -340,6 +357,11 @@ sub handle_options
  }
  local $Exporter::ExportLevel = $state->{export_level};
  import OpenBSD::State;
+}
+
+sub is_interactive
+{
+ return 0;
 }
 
 sub defines
Index: OpenBSD/ProgressMeter/Term.pm
===================================================================
RCS file: /cvs/src/usr.sbin/pkg_add/OpenBSD/ProgressMeter/Term.pm,v
retrieving revision 1.36
diff -u -p -r1.36 Term.pm
--- OpenBSD/ProgressMeter/Term.pm 15 Jun 2016 15:31:09 -0000 1.36
+++ OpenBSD/ProgressMeter/Term.pm 22 Jun 2016 15:55:30 -0000
@@ -127,9 +127,17 @@ sub find_window_size
  my @l = Term::ReadKey::GetTermSizeGWINSZ(\*STDOUT);
  if (@l != 4) {
  $self->{width} = 80;
+ $self->{height} = 24;
  } else {
  $self->{width} = $l[0];
+ $self->{height} = $l[1];
  }
+}
+
+sub height
+{
+ my $self = shift;
+ return $self->{height};
 }
 
 sub compute_playfield

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Ted Unangst-6
In reply to this post by Marc Espie-2
Marc Espie wrote:
> This would allow pkg_add to auto-configure a mirror, for the case where
> PKG_PATH was not specified and where pkg.conf does not exist.
>
> It only triggers when a location ends up empty and when run in interactive
> mode, e.g., it shouldn't interfere with local lookups.
>
> Good idea, or awful ?

This would be pretty surprising to me I think. If for some reason I have
failed to configure a mirror, I would prefer to get an error so I can fix the
underlying problem. If we can't contact a DNS server, we don't fallback on a
list of known public servers.

Auto config at install time is helpful, but this sort of dynamic auto config
violates an important principle: it should be possible to unconfig something.

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Theo de Raadt-2
In reply to this post by Marc Espie-2
I agree with Ted.  This feels very much like building a CDN at the wrong
level, considering how slowly and carelessly the mirrors are updated
at this time.

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Sebastian Benoit-3
In reply to this post by Ted Unangst-6
Ted Unangst([hidden email]) on 2016.06.22 12:25:04 -0400:

> Marc Espie wrote:
> > This would allow pkg_add to auto-configure a mirror, for the case where
> > PKG_PATH was not specified and where pkg.conf does not exist.
> >
> > It only triggers when a location ends up empty and when run in interactive
> > mode, e.g., it shouldn't interfere with local lookups.
> >
> > Good idea, or awful ?
>
> This would be pretty surprising to me I think. If for some reason I have
> failed to configure a mirror, I would prefer to get an error so I can fix the
> underlying problem. If we can't contact a DNS server, we don't fallback on a
> list of known public servers.
>
> Auto config at install time is helpful, but this sort of dynamic auto config
> violates an important principle: it should be possible to unconfig something.

Aside from that, i dont think we should be hardcoding ip-adresses like that.

A name can be changed in DNS, but this will cause http requests to that ip
for quite some time.

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Giovanni Bechis-7
On 06/22/16 18:28, Sebastian Benoit wrote:

> Ted Unangst([hidden email]) on 2016.06.22 12:25:04 -0400:
>> Marc Espie wrote:
>>> This would allow pkg_add to auto-configure a mirror, for the case where
>>> PKG_PATH was not specified and where pkg.conf does not exist.
>>>
>>> It only triggers when a location ends up empty and when run in interactive
>>> mode, e.g., it shouldn't interfere with local lookups.
>>>
>>> Good idea, or awful ?
>>
>> This would be pretty surprising to me I think. If for some reason I have
>> failed to configure a mirror, I would prefer to get an error so I can fix the
>> underlying problem. If we can't contact a DNS server, we don't fallback on a
>> list of known public servers.
>>
>> Auto config at install time is helpful, but this sort of dynamic auto config
>> violates an important principle: it should be possible to unconfig something.
>
> Aside from that, i dont think we should be hardcoding ip-adresses like that.
>
we are doing that with miniroot/install.sub, time to change ?
 
> A name can be changed in DNS, but this will cause http requests to that ip
> for quite some time.
>
I agree with that.
 Cheers
  Giovanni

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Theo de Raadt
> > Aside from that, i dont think we should be hardcoding ip-adresses like that.
> >
> we are doing that with miniroot/install.sub, time to change ?

No, that is not what miniroot/install.sub does.  Not at all.

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Giovanni Bechis-7
On 06/22/16 18:57, Theo de Raadt wrote:
>>> Aside from that, i dont think we should be hardcoding ip-adresses like that.
>>>
>> we are doing that with miniroot/install.sub, time to change ?
>
> No, that is not what miniroot/install.sub does.  Not at all.
>
I know, I just said that the ip address is hardcoded somewhere else as well.

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Theo de Raadt
> On 06/22/16 18:57, Theo de Raadt wrote:
> >>> Aside from that, i dont think we should be hardcoding ip-adresses like that.
> >>>
> >> we are doing that with miniroot/install.sub, time to change ?
> >
> > No, that is not what miniroot/install.sub does.  Not at all.
> >
> I know, I just said that the ip address is hardcoded somewhere else as well.

No, it is not.

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Marc Espie-2
this is the exact same code that s currently in install.sub
transposed ad perl

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Theo de Raadt-2
In reply to this post by Marc Espie-2
>this is the exact same code that s currently in install.sub
>transposed ad perl

I get it.

It makes sense for installing the base system.

We started using it in pkg.conf.  I am no longer sure that is the
right thing to do.

Speaking to the installation backend all the time worries me.

Reply | Threaded
Open this post in threaded view
|

Re: add mirror discovery to pkg_add

Marc Espie-2
In reply to this post by Theo de Raadt-2

You guys made me think about the actual use case: noob user of OpenBSD,
installs the ISO, never gets to have any pkg.conf by default.

A way to handle that case would be to have non-network iso *installs* put
a pkg.conf that says "hey we didn't configure anything, let's do that later".

A bit a la first-time-boot.

If you unconfigure things, end up with no pkg.conf, then it will never trigger.

Anyhow, the code I have made me see thru a few "fun" pkg_add details I'll
have to fix anyway (make ask_list able to deal with long lists in every case
looks like a worthwhile pursuit always)