Andrew Hewus Fresh writes:
> 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. Mmm, good call.. I am going to commit with this version - thanks for the clue sticks! :D > >> - 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
