search

TheThingsArchive / ttn

The Things Network Stack V2
https://www.thethingsnetwork.org
MIT License
461 stars 277 forks source link

Go get fails #719

Closed rikvdh closed 6 years ago

rikvdh commented 6 years ago

Hi,

When I use go get to retrieve or update this repository I get an error:

$ go get github.com/TheThingsNetwork/ttn
# github.com/TheThingsNetwork/ttn/vendor/github.com/brocaar/lorawan/band
ttn/vendor/github.com/brocaar/lorawan/band/band_au915_928.go:10:45: undefined: Band
ttn/vendor/github.com/brocaar/lorawan/band/band_au915_928.go:11:23: undefined: MaxPayloadSize
ttn/vendor/github.com/brocaar/lorawan/band/band_au915_928.go:14:22: undefined: MaxPayloadSize
ttn/vendor/github.com/brocaar/lorawan/band/band_au915_928.go:33:22: undefined: MaxPayloadSize
ttn/vendor/github.com/brocaar/lorawan/band/band_au915_928.go:53:10: undefined: Band
ttn/vendor/github.com/brocaar/lorawan/band/band_au915_928.go:229:28: undefined: Channel
ttn/vendor/github.com/brocaar/lorawan/band/band_au915_928.go:237:31: undefined: Channel
ttn/vendor/github.com/brocaar/lorawan/band/band_au915_928.go:245:30: undefined: Channel

I see why this error is generated, the included vendor is incomplete. I've also seen that this file differs from the file from the original vendor.

This is very odd from a programmers perspective. Why is the file there? Why is it different than the original repository? If it needs to be different, why isn't it backported to the original repository?

htdvisser commented 6 years ago

Please follow the instructions in the README file to clone this repo. We use govendor to deal with dependencies that may break their APIs. The band_au915_928.go contains a couple of specific patches for our Australian users.

xor-gate commented 6 years ago

So let me just quote:

The point is that adding just the vendor directory mechanism to the go command allows other tools to achieve their goals of reproducible builds and not modifying vendored source code while still remaining compatible with plain “go get”. https://github.com/golang/proposal/blob/master/design/25719-go15vendor.md#example

Which basicly means the ttn package doesn't comply and needs some kind of special treatment. It could be easily fixed:

xor-gate commented 6 years ago

I understand changes made to third-party packages, I would like to help you out. But the file is almost completely changed (internal and public APIs). So I have no idea where to start collaborating with @brocaar to get it upstream.

This is the diff https://github.com/brocaar/lorawan/compare/master...dualinventive:ttn-issue-719

brocaar commented 6 years ago

The band_au915_928.go contains a couple of specific patches for our Australian users.

@htdvisser are these patches specific to TTN users, or were these to fix mistakes in the band package?

Note: A while ago I refactored the band package in order to deal with multiple regional parameter revisions. This was needed as within a single network, different devices might implement different Regional Parameters revisions (e.g. max payload size), thus the lorawan/band package needs to be able to return the correct value per revision.

To facilitate this, I have refactored all struct based configuration into an interface specifying the methods that each band configuration must expose. See also: https://github.com/brocaar/lorawan/blob/master/band/band.go#L107.

The move from struct based configuration also makes it possible to add additional logic to methods. E.g. GetDownlinkTXPower (which receives the TX frequency) can now return a different TX power per frequency or per number of enabled channels.

@htdvisser I think it could be interesting for you to migrate to the latest lorawan/band package to make use of these new features.

@xor-gate the diff result is because the diff is not only the patch, but also the change from old to new format.

xor-gate commented 6 years ago

Yeah I was confused by the change in the old and new format. I'm not very familiar with the LoRaWAN spec so it would take to much effort for me to just add some "specific patches".

johanstokking commented 6 years ago

@brocaar we're not migrating as V2 development is discontinued, but otherwise that would be a better idea indeed. The changes are incorporating 1.1 revB, if I'm not mistaken.

I agree it's cleaner to fork the repo and vendor from there. I don't think it'll be useful to submit it upstream.

rikvdh commented 6 years ago

@johanstokking What has V2 development todo with updating to the latest github,com/brocaar/lorawan?

@brocaar When I look at the changes they did it seemed that it is indeed to support 1.1rB regional parameters. For example see the attached pictures.

change capture

Is the latest v1.1rB adopted into brocaar/lorawan?

@htdvisser @johanstokking Would you agree on a PR integrating the latest brocaar/lorawan? I am willing to work on this, since I believe in the fact that cleaner code helps in wider adoption of the application.

rikvdh commented 6 years ago

Oh sorry, I see that when you upgrade the commit to https://github.com/brocaar/lorawan/commit/4b356a03c38466be219afc4f5c068585f75d264a (which is 1 commit later than the current rev aa09ef2) the same changes are incorporated in github.com/brocaar/lorawan.

Perhaps just change the vendor/vendor.json to incorporate the "new" (27-7-2017) commit.

htdvisser commented 6 years ago

The vendor/versioning problem is well known in the Go community. Most people know that the "API stability promise" of a package is only theoretical. In practice packages evolve over time, sometimes resulting in breaking changes.

This was the case with @brocaar's lorawan package, where https://github.com/brocaar/lorawan/commit/aa09ef2fd7be6786b853675bb208c26ceeaf9bfd removed the TxPower that we're using in TTNv2, and therefore breaking our code. At the time we decided to just pin the old version in govendor (our vendoring system) and keep using that. This indeed means that a plain go get flag fails, as it would fetch the newest dependency (the one with the breaking changes). It also meant that we didn't have the newer versions of the band sub-package, which turned out to be problematic for our 1.0.2rB users in Australia.

Ideally, we'd upgrade to the newest version of @brocaar's library, to benefit from the new features he added there. Unfortunately that could cost a lot of time, because of the API changes and the way we use the library at the moment. We'd rather spend that time on our upcoming v3 stack, which is a complete rewrite, so any large v2 changes would just mean more wasted time.

As a quick fix, we decided to explicitly vendor (as in; commit to git) the lorawan and lorawan/band package. This unfortunately still means that you can't go get this repository without using the -d flag, and you can't go install this repository without running govendor sync first.

Please accept this workaround for now. Things should be a lot better when our v3 stack is done 😄