Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please restore default branch to V1 #26

Open
stapelberg opened this issue Nov 14, 2018 · 17 comments
Open

Please restore default branch to V1 #26

stapelberg opened this issue Nov 14, 2018 · 17 comments

Comments

@stapelberg
Copy link
Contributor

stapelberg commented Nov 14, 2018

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
Copy link
Owner

d2g commented Nov 14, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

rgooch commented Nov 16, 2018

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
Copy link
Owner

d2g commented Nov 16, 2018

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
Copy link
Contributor Author

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
Copy link
Owner

d2g commented Nov 16, 2018

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
Copy link
Contributor Author

For modules, that will (eventually) be addressed with golang/go#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
Copy link
Owner

d2g commented Nov 16, 2018

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
Copy link
Contributor Author

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

@stapelberg
Copy link
Contributor Author

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
Copy link
Owner

d2g commented Nov 16, 2018

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
Copy link
Contributor Author

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
Copy link
Owner

d2g commented Nov 16, 2018

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
Copy link

rgooch commented Nov 16, 2018

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
Copy link
Owner

d2g commented Dec 28, 2018

@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
Copy link
Contributor Author

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants