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

Use patched go-ethereum from fork instead of patching it on-the-fly #1184

Merged
merged 8 commits into from
Sep 27, 2018

Conversation

adambabik
Copy link
Contributor

@adambabik adambabik commented Sep 5, 2018

Instead of patching code after running dep ensure, use a patched fork.

The problem I encountered is that it's impossible to use status-go as a package due to patches on its dependency, mainly github.com/ethereum/go-ethereum. Even if dep is able to resolve dependencies, compiling will fail due to code differences between github.com/ethereum/go-ethereum and patched version that status-go uses.

A proposed solution is to use change source of github.com/ethereum/go-ethereum in Gopkg.toml to our fork:

[[constraint]]
  name = "github.com/ethereum/go-ethereum"
  version = "=v1.8.14"
  source = "github.com/status-im/go-ethereum"

After this operation, I am able to create a new project with the following Gopkg.toml:

[[constraint]]
  name = "github.com/ethereum/go-ethereum"
  version = "=v1.8.14"
  source = "github.com/status-im/go-ethereum"

[[constraint]]
  name = "github.com/status-im/status-go"
  version = "=v0.14.1"

# I skipped some additional issues with go-ethereum like pruning non-go packages.

Todo

  • remove vendor-check and update-geth,
  • change patch and patch-revert so they work on our go-ethereum fork,
  • update readme in _assets/patches/geth.

@adambabik adambabik changed the title Use patched go-ethereum from fork instead of patching it on-the-fly [WIP] Use patched go-ethereum from fork instead of patching it on-the-fly Sep 5, 2018
@divan
Copy link
Contributor

divan commented Sep 7, 2018

I'm generally now more in favour of this approach because it will help in switching to Go modules, but I don't really get the moment of:

is that it's impossible to use status-go as a package due to patches on its dependency

Why it's not impossible? We're not patching on-the-fly, only when bumping up go-ethereum version, right? Or am I missing something?

@adambabik
Copy link
Contributor Author

@divan because if you use github.com/status-im/status-go as a dependency, it implicitly requires github.com/ethereum/go-ethereum as well, however, it's not taken from github.com/status-im/status-go's vendor directory which is the only place where it's patched. Instead, it gets it from github.com/ethereum/go-ethereum so you end up with non-patched code.

@adambabik adambabik changed the title [WIP] Use patched go-ethereum from fork instead of patching it on-the-fly Use patched go-ethereum from fork instead of patching it on-the-fly Sep 13, 2018
@adambabik adambabik requested review from mandrigin, pedropombeiro and divan and removed request for mandrigin and pedropombeiro September 13, 2018 07:49
Copy link
Contributor

@mandrigin mandrigin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that the research team is aware of this changes so they don’t break the fork repo.

@dshulyak
Copy link
Contributor

dshulyak commented Sep 13, 2018

@adambabik if fork is patched instead of vendored copy, what is the reason to keep patches in status-go?

@adambabik
Copy link
Contributor Author

@dshulyak it's actually hard to figure out a good place for the patches in our fork. We can have a special branch in our fork where patches could live. Now, if a new version appears, we would checkout them from that branch and apply on the new version branch. It seems like the same effort.

But primarily, I wanted to do as few changes as possible. When using a fork, we can can also think about getting rid of the patches completely. We could have patched/1.8 long-running branch with our changes and merge new versions on top of it like this git merge upstream/v1.8.15.

@adambabik adambabik merged commit ac8da3c into develop Sep 27, 2018
@adambabik adambabik deleted the deps/use-patched-go-ethereum branch September 27, 2018 19:16
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

Successfully merging this pull request may close these issues.

5 participants