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;
>> }
>>
No comments:
Post a Comment