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