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? > Since I need to know the version to get the distfiles - I have to do the check early on in get_dist_info - then later portgen calls get_ver_info. Currently I am caching the version in $self->{version_info} and short circuiting get_ver_info if it's set. I actually want the failure to happen in get_ver_info, because at that point we really can't determine what we need to download. Hope that makes sense :D > Perhaps `get_ver_info` should be: > > sub get_ver_info { > my ($self, $module) = @_; > > return $self->{ver_info} if $self->{ver_info}; > > my $ver_info = do { local $@; eval { local $SIG{__DIE__}; > $self->_get_latest_ver_info($module) } }; > > unless ($version) { > my $version = $self->_get_versions($module)->[0]; > croak("Unable to find a version for $module") > unless $version; > $ver_info = { Module => $module, Version => $version }; > } > > return $self->{ver_info} = $ver_info; > } > > >> + $json //= $self->get_ver_info($module); >> >> if ($module =~ m/v\d$/) { >> $json->{Name} = ( split '/', $module )[-2]; >> @@ -81,6 +78,7 @@ sub _go_determine_name >> } >> >> $json->{Module} = $module; >> + $self->{version_info} = $json; >> >> return $json; >> } >> @@ -90,6 +88,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 +132,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,6 +212,10 @@ sub _go_mod_normalize >> sub get_ver_info >> { >> my ( $self, $module ) = @_; >> + >> + # We already ran, skip re-running. >> + return $self->{version_info} if defined $self->{version_info}; >> + >> my $version_list = $self->get( $module . '/@v/list' ); >> my $version = "v0.0.0"; >> my $ret; >> @@ -227,13 +230,15 @@ sub get_ver_info >> 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; >> } >> } > > I think this Schwartzian transform is a bit easier for me to understand > what is going on, but either way this is a good bugfix. > > Index: lib/OpenBSD/PortGen/Port/Go.pm > =================================================================== > RCS file: /cvs/ports/infrastructure/lib/OpenBSD/PortGen/Port/Go.pm,v > retrieving revision 1.7 > diff -u -p -r1.7 Go.pm > --- lib/OpenBSD/PortGen/Port/Go.pm 16 Jan 2021 23:38:13 -0000 1.7 > +++ lib/OpenBSD/PortGen/Port/Go.pm 27 Jan 2021 02:42:30 -0000 > @@ -223,14 +223,10 @@ sub get_ver_info > if ($version_list eq "") { > $ret = $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)) { > - $version = $v; > - } > - } > + ($version) = map { $_->[0] } > + sort { $b->[1]->compare($a->[1]) } > + map { [ $_, OpenBSD::PackageName::version->from_string($_) } > + split("\n", $version_list); > $ret = { Module => $module, Version => $version }; > } > > > >> $ret = { Module => $module, Version => $version }; >> } >> >> + $self->{version_info} = $ret; >> + >> return $ret; >> } >>
