Panic in semantic version parsing

[dswarbrick]

Debian sometimes like to append git commit IDs to package versions, and this is resulting in a panic when Unsee tries to parse the version:

INFO[0000] GET http://fra-alertmgr:9093/api/v1/status timeout=40s 
INFO[0000] [default] Remote Alertmanager version: 0.15.0~rc.1~git20180507.28967e3+ds 
panic: semver: Parse(0.15.0~rc.1~git20180507.28967e3+ds): Invalid character(s) found in patch number "0~rc.1~git20180507.28967e3"

goroutine 11 [running]:
github.com/cloudflare/unsee/vendor/github.com/blang/semver.MustParse(0xc420025440, 0x22, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /home/travis/gopath/src/github.com/cloudflare/unsee/vendor/github.com/blang/semver/semver.go:319 +0x20c
github.com/cloudflare/unsee/internal/mapper/v04.SilenceMapper.IsSupported(0x0, 0x0, 0xc420025440, 0x22, 0xc4201066a0)
        /home/travis/gopath/src/github.com/cloudflare/unsee/internal/mapper/v04/silences.go:67 +0x70
github.com/cloudflare/unsee/internal/mapper.GetSilenceMapper(0xc420025440, 0x22, 0xc42002e500, 0xc420065418, 0xc420065420, 0xc4200907d0)
        /home/travis/gopath/src/github.com/cloudflare/unsee/internal/mapper/mapper.go:59 +0xa1
github.com/cloudflare/unsee/internal/alertmanager.(*Alertmanager).pullSilences(0xc420161110, 0xc420025440, 0x22, 0x0, 0x0)
        /home/travis/gopath/src/github.com/cloudflare/unsee/internal/alertmanager/models.go:107 +0x79
github.com/cloudflare/unsee/internal/alertmanager.(*Alertmanager).Pull(0xc420161110, 0x23, 0xc420040f90)
        /home/travis/gopath/src/github.com/cloudflare/unsee/internal/alertmanager/models.go:300 +0x6d
main.pullFromAlertmanager.func1(0xc420026f20, 0xc420161110)
        /home/travis/gopath/src/github.com/cloudflare/unsee/timer.go:25 +0xab
created by main.pullFromAlertmanager
        /home/travis/gopath/src/github.com/cloudflare/unsee/timer.go:23 +0x10e

I realize that this is technically not a bug in Unsee, rather in blang/semver, but perhaps Unsee could offer a workaround?

[terinjokes]

Updating the version in packaging metadata is one thing, but why is Debian modifying upstream's version while compiling?

[dswarbrick]

In the debian/rules file, various build flags are overridden with local values:

BUILDFLAGS := -ldflags \
  " -X $(METAPKG)/version.Version=$(DEB_VERSION_UPSTREAM)\
    -X $(METAPKG)/version.Revision=$(DEB_VERSION)\
    -X $(METAPKG)/version.Branch=$(BRANCH)\
    -X $(METAPKG)/version.BuildUser=$(USER)\
    -X $(METAPKG)/version.BuildDate=$(BUILD_DATE)\
    -X $(METAPKG)/version.GoVersion=$(GO_VERSION)"

This in itself is curious, because variables such a $USER and $BUILD_DATE will break Debian's attempts to ensure reproducible builds.... Although the Prometheus developers have to assume some of the blame on that one, for including such obviously changeable metadata in an end-result binary.

[NightTsarina]

Hey,
I am the Debian maintainer for the alertmanager. Just chiming in to explain why the changes I do in the metadata.

The main reason to change these fields is actually reproducibility. Both USER and BUILD_DATE are reproducible because they are set in the debian/rules makefile. Also, it is important to show this is a Debian release, so upstream does not have to deal with bugs that are Debian's responsibility.

The tilde itself is added because this is an release candidate. 0.15-rc.1 would sort as higher than 0.15, so we don't want that. The tilde sorts in a special way such that 0.15~rc.1 is lower than 0.15, but this is important only for the package version. If there is a way to have semver interpret the RC correctly, i can change the metadata that is exposed in the API to fix this issue.

[terinjokes]

@TheTincho I don't disagree that you could (and should) use the additional metadata in packaging, I was just surprised to see it passed through to the binary. Minor differences in packaging beliefs, no big deal.

I've submitted a CL that trims everything after the tilde before parsing as a semver. I'm happy to extend that to other forms for other distributions.

To answer your question, the semver way to encode this would be "0.15.0-rc.1", which is earlier than "0.15.0". Unfortunately, this conflicts with debian-revision, thus your tilde.

Hopefully we'll never have to worry about epochs.

[NightTsarina]

Hi Terin, thanks for replying.

I checked the CL, and I think that patch is not a good idea, because the tilde is not a packaging version. That is what is after the dash in the Debian standard: 0.15.0~rc.2-1 is the full version, where -1 is the packaging version and 0.15.0~rc.2 is the (mangled) upstream version.

If you wanted to support this in semver, the correct way would be to interpret it as a pre-release, just like with the dash.

You don't need to worry about epochs (because that is also packaging only), and I can just de-mangle the upstream version to -rc.2. I had not thought before anybody would be parsing this, and knowing now how semver interprets it, I think it is the proper thing to do.

[terinjokes]

@TheTincho If you don't mind making that change, I can abandon the CL.

@dswarbrick How does this sound for you?

[NightTsarina]

I have just uploaded another snapshot with this change.

[dswarbrick]

I'm testing @TheTincho's 0.15.0~rc.2+ds-1 package, and Unsee is working fine with this.
LGTM.

[terinjokes]

I'm going to go ahead and close this and abandon the CL.