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

investigate replace in go.mod instead of renaming all imports #57

Closed
liamsi opened this issue Sep 9, 2020 · 5 comments
Closed

investigate replace in go.mod instead of renaming all imports #57

liamsi opened this issue Sep 9, 2020 · 5 comments
Assignees
Labels
T:investigate Further investigation needed T:tendermint Type: upstream tendermint changes we want

Comments

@liamsi
Copy link
Member

liamsi commented Sep 9, 2020

Can we avoid merge conflicts and decrease maintenance costs for updating to upstream (tendermint) master by using replace?

@liamsi liamsi added T:question Further information is requested T:tendermint Type: upstream tendermint changes we want T:investigate Further investigation needed and removed T:question Further information is requested labels Sep 9, 2020
@liamsi liamsi self-assigned this Sep 9, 2020
@liamsi
Copy link
Member Author

liamsi commented Sep 9, 2020

One observation is that tendermint (and hence ll-core) uses some internal package(s) (e.g. github.com/tendermint/tendermint/tools/tm-signer-harness/internal or github.com/tendermint/tendermint/crypto/internal/benchmarking) which doesn't seem to work well with the replace directive.
E.g.,

package github.com/lazyledger/lazyledger-core/tools/tm-signer-harness
        tools/tm-signer-harness/main.go:14:2: use of internal package github.com/tendermint/tendermint/tools/tm-signer-harness/internal not allowed

We might be able to work around this though.

But even if I remove all packages (it's mostly test packages, eg. the tm-signer-harness and the bench_tests for the sig libs), there are still a lot of errors. I'll push a branch with my findings so far.

@liamsi
Copy link
Member Author

liamsi commented Sep 9, 2020

@liamsi
Copy link
Member Author

liamsi commented Sep 9, 2020

TBH, I don't think replace is meant to be used like this (replace the module that you are defining), only to replace dependencies (which could be forks).

@liamsi
Copy link
Member Author

liamsi commented Sep 9, 2020

Also, to quote a go core dev:

replace is intended to support local development, not permanent forks: if you need to permanently fork a dependency, your fork must have its own, unique import path.

@liamsi
Copy link
Member Author

liamsi commented Sep 9, 2020

Closing for now.

@liamsi liamsi closed this as completed Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:investigate Further investigation needed T:tendermint Type: upstream tendermint changes we want
Projects
None yet
Development

No branches or pull requests

1 participant