On Sat, Jan 30, 2021 at 01:20:25PM -0700, Aaron Bieber wrote:
>
> Aaron Bieber writes:
>
> > Andrew Hewus Fresh writes:
> >
> >> On Tue, Jan 26, 2021 at 05:23:30PM -0700, Aaron Bieber wrote:
> >>> Hi,
> >>>
> >>> Recently a program was found that caused breakage in 'portgen go'. The
> >>> breakage was two fold:
> >>>
> >>> 1) https://proxy.golang.org/qvl.io/promplot/@latest returns unexpected
> >>> results. This caused portgen to bomb out.
> >>> 2) Even it 1) had worked, the logic in 'get_ver_info' was broken and it
> >>> picked the wrong version.
> >>>
> >>> This diff changes things so we first try to get our version from
> >>> '/@latest'. If that fails, we call `get_ver_info` which pulls down the
> >>> full list of versions from the '/@v/list' endpoint.
> >>>
> >>> Thanks to afresh1 for showing me the neat '//=' perl trick!
> >>>
> >>> Tested with:
> >>> portgen go qvl.io/promplot
> >>>
> >>> as well as a number of other ports.
> >>>
> >>> OK? Cluesticks?
> >>>
> >>
> >> This seems OK to me, a couple of comments though, but it's up to you
> >> whether you change anything.
> >>
> >>
> >>> diff 6a862af059a42a1899fe9a62461b2acfc0f8eedc /usr/ports
> >>> blob - 89f2c7297409ef9d54fd1bdde73cf1829c742ff3
> >>> file + infrastructure/lib/OpenBSD/PortGen/Port/Go.pm
> >>> --- infrastructure/lib/OpenBSD/PortGen/Port/Go.pm
> >>> +++ infrastructure/lib/OpenBSD/PortGen/Port/Go.pm
> >>> @@ -67,12 +67,9 @@ sub _go_determine_name
> >>> # Some modules end in "v1" or "v2", if we find one of these, we need
> >>> # to set PKGNAME to something up a level
> >>> my ( $self, $module ) = @_;
> >>> - my $json = $self->get_json( $self->_go_mod_normalize($module) .
> >>> '/@latest' );
> >>>
> >>> - if ($json->{Version} =~ m/incompatible/) {
> >>> - my $msg = "${module} $json->{Version} is incompatible with Go
> >>> modules.";
> >>> - croak $msg;
> >>> - }
> >>
> >> Do you still need to check for "incompatible" someplace?
> >
> > As mentioned in irc - this is leftover and we don't actually hit the
> > condition anyway.
> >>
> >>
> >>> + my $json = eval { $self->get_json( $self->_go_mod_normalize($module) .
> >>> '/@latest' ) };
> >>
> >> Should this eval'd check for `@latest` be in `get_ver_info`?
> >> If not, why not?
> >>
>
> Here is a version that moves all the version checks into
> get_ver_info. Really it just removes the duplicate I had >.>
>
> diff 9ae2711eb3404621b05283a43f79a65b656ddf47 /usr/ports
> blob - 89f2c7297409ef9d54fd1bdde73cf1829c742ff3
> file + infrastructure/lib/OpenBSD/PortGen/Port/Go.pm
> --- infrastructure/lib/OpenBSD/PortGen/Port/Go.pm
> +++ infrastructure/lib/OpenBSD/PortGen/Port/Go.pm
> @@ -67,12 +67,8 @@ sub _go_determine_name
> # Some modules end in "v1" or "v2", if we find one of these, we need
> # to set PKGNAME to something up a level
> my ( $self, $module ) = @_;
> - my $json = $self->get_json( $self->_go_mod_normalize($module) .
> '/@latest' );
>
> - if ($json->{Version} =~ m/incompatible/) {
> - my $msg = "${module} $json->{Version} is incompatible with Go
> modules.";
> - croak $msg;
> - }
> + my $json = $self->get_ver_info($module);
>
> if ($module =~ m/v\d$/) {
> $json->{Name} = ( split '/', $module )[-2];
> @@ -90,6 +86,7 @@ sub get_dist_info
> my ( $self, $module ) = @_;
>
> my $json = $self->_go_determine_name($module);
> +
> my ($dist, $mods) = $self->_go_mod_info($json);
> $json->{License} = $self->_go_lic_info($module);
>
> @@ -133,7 +130,7 @@ sub _go_mod_info
>
> my $mod = $self->get($self->_go_mod_normalize($json->{Module}) .
> "/\@v/$json->{Version}.mod");
> my ($module) = $mod =~ /\bmodule\s+(.*?)\s/;
> -
> +
> unless ( $json->{Module} eq $module ) {
> my $msg = "Module $json->{Module} doesn't match $module";
> croak $msg;
> @@ -213,28 +210,36 @@ sub _go_mod_normalize
> sub get_ver_info
> {
> my ( $self, $module ) = @_;
> - my $version_list = $self->get( $module . '/@v/list' );
> +
> + # We already ran, skip re-running.
> + return $self->{version_info} if defined $self->{version_info};
> +
> + my $json;
> + my $version_list = eval { $self->get( $module . '/@v/list' ) };
My understanding of go version numbers is that "0" is not a valid
version, which means that all valid versions will be truthy. If that
understanding is wrong, then this code is probably wrong, but if true
that means you don't need the `//= ""` because you can just test
$version_list in boolean context directly.
Here's an untested suggestion of how I'd write get_ver_info.
(this also avoids re-parsing what we think is the latest version
multiple times)
my $version_list = do { local $@; eval { local $SIG{__DIE__};
$self->get( $module . '/@v/list' ) } };
my $version_info;
if ($version_list) {
my %v = ( o =>
OpenBSD::PackageName::version->from_string("v0.0.0") );
for my $v ( split "\n", $version_list ) {
my $o = OpenBSD::PackageName::version->from_string($v);
if ( $v{o}->compare($o) == -1 ) {
%v = ( Version => $v, o => $o );
}
}
if ($v{Version}) {
$version_info = { Module => $module, Version =>
$v{Version} };
}
else {
croak "Unable to determine version for $module!";
}
}
else {
$version_info = $self->get_json(
$self->_go_mod_normalize($module) . '/@latest' );
}
return $self->{version_info} = $version_info;
> my $version = "v0.0.0";
This $version variable only needs to live inside the "else" block below,
it's not used anywhere else. Or you could go with something like the
above.
> - my $ret;
>
> + $version_list //= "";
> +
> # If list isn't populated, it's likely that upstream doesn't follow
> # semver, this means we will have to fallback to @latest to get the
> # version information.
> if ($version_list eq "") {
> - $ret = $self->get_json( $self->_go_mod_normalize($module) .
> '/@latest' );
> + $json = $self->get_json( $self->_go_mod_normalize($module) .
> '/@latest' );
> } else {
> my @parts = split("\n", $version_list);
> for my $v ( @parts ) {
> my $a =
> OpenBSD::PackageName::version->from_string($version);
> my $b = OpenBSD::PackageName::version->from_string($v);
> - if ($a->compare($b)) {
> + if ($a->compare($b) == -1) {
> $version = $v;
> }
> }
> - $ret = { Module => $module, Version => $version };
> + $json = { Module => $module, Version => $version };
> }
>
> - return $ret;
> + $self->{version_info} = $json;
> +
> + return $json;
> }
>
> sub name_new_port
--
andrew - http://afresh1.com
Full-time system administration is a delicate balance
between proactiveness and laziness.
-- jhorwitz from use.perl.org
