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

Change the import rather than relying on the replace directive for dtls modifications #226

Open
cohosh opened this issue Sep 13, 2023 · 3 comments

Comments

@cohosh
Copy link
Contributor

cohosh commented Sep 13, 2023

There is a quirk with Go's replace directive where it is only applied if the directive appears in the go.mod file of the main program, but not if it is in the go.mod file of a dependency.

In a scenario where another Go program is using the conjure/pkg/registration library (as in the Tor Conjure PT), a build will fail with the following errors:

# github.com/refraction-networking/conjure/pkg/dtls
../../../../go/pkg/mod/github.com/refraction-networking/[email protected]/pkg/dtls/dial.go:64:3: unknown field CustomClientHelloRandom in struct literal of type "github.com/pion/dtls/v2".Config
../../../../go/pkg/mod/github.com/refraction-networking/[email protected]/pkg/dtls/listener.go:63:24: connState.RemoteRandomBytes undefined (type "github.com/pion/dtls/v2".State has no field or method RemoteRandomBytes)
../../../../go/pkg/mod/github.com/refraction-networking/[email protected]/pkg/dtls/listener.go:137:34: state.RemoteRandomBytes undefined (type *"github.com/pion/dtls/v2".State has no field or method RemoteRandomBytes)
../../../../go/pkg/mod/github.com/refraction-networking/[email protected]/pkg/dtls/listener.go:236:40: clientHello.RandomBytes undefined (type *"github.com/pion/dtls/v2".ClientHelloInfo has no field or method RandomBytes)

Because the directive

replace github.com/pion/dtls/v2 => github.com/mingyech/dtls/v2 v2.0.0

fails to apply.

This can be fixed by adding the directive to the go.mod file of the calling program, but it would be cleaner to just change the import where it's used, especially since this is a long-term change.

This isn't urgent, just a nice to have :)

@jmwample
Copy link
Member

We are working on a better solution to this issue. The decision to use the replace over modified imports was made so that we wouldn't have to fork (and keep up to date) more of the pion modules relating to dtls. The use of the replace is intended to be a temporary fix.

The plan is to attempt to upstream our changes to the respective pion repositories, at which point the replace would not be necessary at all. Failing that though, we will fork the remaining pion libraries and switch to imports.

@cohosh
Copy link
Contributor Author

cohosh commented Sep 13, 2023

Thanks for the update! That sounds like a good plan.

@mingyech
Copy link
Contributor

pion/dtls#583 should let us remove the replace if it gets merged! The remaining replace for pion/transport will be unneeded and removed as well.

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