d2g / dhcp4client

DHCP Client
Mozilla Public License 2.0
38 stars 30 forks source link

Please restore default branch to V1 #26

Open stapelberg opened 5 years ago

stapelberg commented 5 years ago

The current default branch of V2 broke e.g. github.com/gokrazy/gokrazy/cmd/dhcp:

% go get -u github.com/gokrazy/gokrazy/cmd/dhcp
# github.com/gokrazy/gokrazy/cmd/dhcp
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:60:14: cannot use &discoveryPacket (type *"github.com/krolaw/dhcp4".Packet) as type *"github.com/d2g/dhcp4".Packet in argument to addHostname
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:61:14: cannot use &discoveryPacket (type *"github.com/krolaw/dhcp4".Packet) as type *"github.com/d2g/dhcp4".Packet in argument to addClientId
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:64:13: c.SendPacket undefined (type *dhcp4client.Client has no field or method SendPacket)
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:65:3: cannot use discoveryPacket (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:70:3: cannot use offerPacket (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:74:14: cannot use &requestPacket (type *"github.com/krolaw/dhcp4".Packet) as type *"github.com/d2g/dhcp4".Packet in argument to addHostname
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:75:14: cannot use &requestPacket (type *"github.com/krolaw/dhcp4".Packet) as type *"github.com/d2g/dhcp4".Packet in argument to addClientId
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:78:13: c.SendPacket undefined (type *dhcp4client.Client has no field or method SendPacket)
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:79:3: cannot use requestPacket (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:84:3: cannot use acknowledgement (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument
/tmp/gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:84:3: too many errors
d2g commented 5 years ago

I understand things are going to break. I'm happy for any suggestions on a way to move this repo forward. The implementation at the time was to meet a minimum requirement I had at the time. It's evident that it has a variety of issues with the design due to my understanding of DHCP. Even now my understanding of DHCP is getting better as I look into and resolve issues.

Again open to suggestions?

stapelberg commented 5 years ago

My suggestion is: set the default branch to V1 so that go get keeps working.

Then, whenever you break the public API, increase the major version.

See also https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher, which is applicable to Go modules (which might be a good idea to start supporting), but also makes things strictly better for users who don’t use modules yet.

stapelberg commented 5 years ago

Friendly ping? I’d like to unbreak my code. Have a look at https://help.github.com/articles/setting-the-default-branch/ if you haven’t dealt with the default branch setting before :)

Thanks!

rgooch commented 5 years ago

I just ran into this problem too: it's broken our TravisCI build: cmd/installer/configureLocalNetwork.go:56:24: undefined: dhcp4client.NewPacketSock cmd/installer/configureLocalNetwork.go:79:6: cannot use packet (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument

d2g commented 5 years ago

So reading through the document suggested I've tagged the tip of V1 with the v1.0.0 tag. The V1 branch exists for maintenance.

Is this working with go.mod or is there something more I need to do?

stapelberg commented 5 years ago

Thanks for the tag, that should help with the go.mod use-case.

However, the “go get” (without modules) use-case is still broken:

% mkdir /tmp/gp
% export GOPATH=/tmp/gp
% go get -u github.com/gokrazy/gokrazy/cmd/dhcp
# github.com/gokrazy/gokrazy/cmd/dhcp
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:60:14: cannot use &discoveryPacket (type *"github.com/krolaw/dhcp4".Packet) as type *"github.com/d2g/dhcp4".Packet in argument to addHostname
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:61:14: cannot use &discoveryPacket (type *"github.com/krolaw/dhcp4".Packet) as type *"github.com/d2g/dhcp4".Packet in argument to addClientId
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:64:13: c.SendPacket undefined (type *dhcp4client.Client has no field or method SendPacket)
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:65:3: cannot use discoveryPacket (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:70:3: cannot use offerPacket (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:74:14: cannot use &requestPacket (type *"github.com/krolaw/dhcp4".Packet) as type *"github.com/d2g/dhcp4".Packet in argument to addHostname
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:75:14: cannot use &requestPacket (type *"github.com/krolaw/dhcp4".Packet) as type *"github.com/d2g/dhcp4".Packet in argument to addClientId
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:78:13: c.SendPacket undefined (type *dhcp4client.Client has no field or method SendPacket)
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:79:3: cannot use requestPacket (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:84:3: cannot use acknowledgement (type "github.com/krolaw/dhcp4".Packet) as type "github.com/d2g/dhcp4".Packet in return argument
gp/src/github.com/gokrazy/gokrazy/cmd/dhcp/dhcp.go:84:3: too many errors

Could you also change the default branch please?

d2g commented 5 years ago

The issue with that is things like godoc will point back to the default branch. When I look at repositories I expect TIP to be the latest not a branch not being actively developed.

Fork the repo and point to the fork?

Happy to compromise and give you a notice period if that helps? I'll switch it back until the Mid December?

stapelberg commented 5 years ago

For modules, that will (eventually) be addressed with https://github.com/golang/go/issues/26827.

Currently, if you’d like to use multiple branches in the same repository, you’ll have to use http://labix.org/gopkg.in.

Specifically: you should rename the V2 branch to v2 (lowercase), then use https://gopkg.in/d2g/dhcp4client.v2 to refer to it. This will work with godoc.org, go get, go modules, etc. The default branch should be set to v1 to keep the status quo working (ideally indefinitely). You can add a statement to the README pointing to https://github.com/d2g/dhcp4client/tree/v2.

d2g commented 5 years ago

The default branch should be set to v1 to keep the status quo working (ideally indefinitely).

I don't see other Repos doing this? I don't see this as being the standard approach where you're redirected to a different branch from default from the readme.

stapelberg commented 5 years ago

An example of this practice is https://github.com/sorcix/irc. I’m sure I can find others :)

stapelberg commented 5 years ago

Note that a number of other repositories don’t face the same issue because they have been using gopkg.in from the very beginning, so their existing import paths will be pointed to the (e.g.) v1 branch via gopkg.in. The situation with d2g/dhcp4client is different because the existing import paths don’t contain a reference to any branch.

d2g commented 5 years ago

As you've pointed out this only works for projects using gopkg.in. A number of other repositories went the vendor route.

Repositories without a version are expected to be v0 and therefore unstable. I'm trying to be as friendly as possible if by the book the default position should be I should just push v2 directly into the v1 branch.

I've renamed the branches.

It breaks the standard toolset, godoc no longer details the latest repos. I have no intention of answering this using gopkg.in as it's outside the "standard". If there is an alternative to gopkg.in in the standard go offering I'll happily look at it (like the v1, v2, tagging modules concepts). To help ease this pain.

The default will remain v1 until mid December where I will revert the change back to v2 so that godoc etc works to give the API for the latest version. Maybe the tooling will have caught up by then to not make it an issue. Hopefully that's enough time for to resolve these issues?

Anything else you want me to do, let me know but v2 has to be the version with the "advantages" related to being the default branch.

stapelberg commented 5 years ago

Thanks for changing the default branch, I can confirm that this fixes the immediate issue.

The default will remain v1 until mid December where I will revert the change back to v2 so that godoc etc works to give the API for the latest version. Maybe the tooling will have caught up by then to not make it an issue.

I’m not 100% sure which tooling you refer to, but changing the API of the github.com/d2g/dhcp4client import path remains a breaking change, as per the “import compatibility rule” outlined in https://research.swtch.com/vgo-import.

As you mention, there’s no expectation of backwards-compatibility with unversioned packages, so the decision is ultimately up to you. From my perspective, the friendlier way for the ecosystem is to use gopkg.in and leave the default branch as-is.

Hopefully that's enough time for to resolve these issues?

It sure is enough time, thanks. I’ll switch my code to use the now-working gopkg.in/d2g/dhcp4client.v2 (thanks for the branch rename!), as I need a way to refer to the v2 branch (now and after you switch the default branch). You might want to give your other users a heads-up about the breaking change.

d2g commented 5 years ago

Great. The v2 branch has been coming for some time. It's not yet stable (I'm adding the missing features from the issues and pull requests), essentially it's tip but is close to where the v1 branch is functionally and the changes allow me to address some of the outstanding issues (Unicast Renews etc). I really was using this "stunt" to start this conversation and start trying to get peoples buy-in to the v2 branch.

.

rgooch commented 5 years ago

Thanks for changing the default branch. This unblocks us for now. I'm happy to migrate to v2, once it has the features I need. When will NewPacketSock be available in v2? Or is there a different API that I can use for the same effect? I'm using it in the dhcpRequest function in: https://github.com/Symantec/Dominator/blob/master/cmd/installer/configureLocalNetwork.go

d2g commented 5 years ago

@rgooch I've made some wrappers to help transition. I've checkout your code and I have to change the import on the dhcp4 as I'm now directly using "github.com/krolaw/dhcp4" rather than my own fork (Not sure if this is a good idea or not). It builds ok (cross compiling etc). Let me know how you get on.

@stapelberg How are you getting on?

stapelberg commented 5 years ago

@stapelberg How are you getting on?

Thanks for checking in. I couldn’t figure out how to use the dhcp4client.v2 API and decided to instead switch to a bunch of simple glue functions that tie together gopacket and mdlayher/raw, which I already use elsewhere in my project (https://github.com/rtr7/dhcp4).