Apply geth patches on the fly #653 #660

canercidam · 2018-02-13T13:14:20Z

This PR adds -c option to patcher, which checks for unapplied patches (see comment in script) and points go-ethereum dependency to github.com/ethereum/go-ethereum. Also, a isolate-vendor-check.sh script is added to avoid conflicts between patching and validate-vendor.sh.

Changes:

make vendor-check fails if any of the added patches were not applied in current workspace and branch.
go-ethereum points to github.com/ethereum/go-ethereum.
dep-ensure was created as described in Apply geth patches on the fly. #653.
Custom fork of go-ethereum is not used anywhere.
README.md and DEPENDENCIES.md are updated with new process.

Closes #653

@mandrigin @adambabik

mandrigin · 2018-02-13T15:25:42Z

@canercidam can you point go-ethereum link to 1.7 branch? master looks too different from what we have now. It would be nice to have it updated separately.

canercidam · 2018-02-13T19:23:26Z

CI passed. Important notes:

dep-ensure changes patching state by applying.
vendor-check does not change patching state. Expects all new patches to be applied first, then does vendor check in an isolated manner (all in a temporary branch - reverts all patches before checking).
If waiting for make dep-ensure is not desirable, patch and patch-revert targets are available. patch already reverts and applies again but I kept patch-revert since it's sometimes very useful on command line.

I think this PR needs an initial review before updating README.md and DEPENDENCIES.md. I'll update both with the last state it's agreed on.

@mandrigin @themue @adambabik @divan

mandrigin

Great job! That should simplify our workflow a lot.

A few questions:

what would happen to vendor-check if we remove patches?
what would happen to vendor-check if the patch was changed but the changes didn't apply?

mandrigin · 2018-02-14T08:24:29Z

_assets/patches/patcher

+			done
+		))
+		newPatches=( "${fromDevelop[@]}" "${uncommitted[@]}" )
+		# Checks new patches and exits with 1 if there are unapplied.


what would happen if we removed a patch?

Nothing problematic. Looks like there's a flaw about that - ideally, it should exclude the removed ones - but it doesn't affect the result. Any patch change (addition, removal) is detected as a new patch but it's not counted if it's currently not residing as an unapplied patch in the directory.

Another question is, what happens if there's a non-new & unapplied patch in the directory (since it will get ignored) but that's not a big problem because make dep-ensure keeps things on track.

mandrigin · 2018-02-14T08:26:15Z

_assets/ci/isolate-vendor-check.sh

+
+branchName="$(git rev-parse --abbrev-ref HEAD)"
+
+git checkout -b isolated-vendor-check


I'd add some timestamp to the temp branch name, to avoid conflicts. Maybe isolated-vendor-check/<timestamp>

pedropombeiro

Just a few minor questions (and of course README.md still needs to be updated). This would have saved me probably a full day when I was recently patching our fork for Whisper v6. Looking forward to having this merged in. Great job!

pedropombeiro · 2018-02-14T08:39:53Z

Gopkg.lock

    "whisper/whisperv5"
  ]
-  revision = "5b2cc44bf2b32bb482def02d7c8fa32ba08d0bf4"
-  source = "https://github.com/status-im/go-ethereum.git"
+  revision = "4bb3c89d44e372e6a9ab85a8be0c9345265c763a"


Shouldn't we run dep ensure -vendor-only so that the vendor sources which are committed with the repo match the revision? E.g.:

Can you elaborate?
We won't be able to keep the same revision SHA in this PR because we point to another repository.
In this diff I don't see anything changed in go-ethereum, just one main file + a few test utils.

Nevermind, I'm still partly asleep :-) I was assuming that the vendor folder would contain the unpatched version, which of course doesn't make sense.

pedropombeiro · 2018-02-14T08:42:46Z

_assets/ci/isolate-vendor-check.sh

+# 1) Stashes all changes and checks out to a temporary branch.
+# 2) Reverts all patches and commits changes.
+# 3) Runs "dep ensure" and validate-vendor.sh. Saves exit code and message.
+# 4) Commits any changes.


Why do we commit the changes if the branch is going to be removed in the next step? Is that for debugging purposes in case the process fails?

About first commit: Running the validation script when there are patches, it says "Oh, you've made some changes! That's not the way to work with dep." Then it removes all our patches and complains until we commit, which is bad because now it removed all of the changes it's not possible to check any other changes. And this script is about checking those ones because ideally there should be no other changes than patches. After reversion and committing, what's left are those.

About second commit: It rarely leaves the workspace in an uncommitted state so that's useful before git checkout <previous_branch>.

Yeah, I was just wondering what was the reason to not do git reset --hard instead.

That was my first option but this looked like a cleaner approach than counting successful commits. And safer while making changes on the script.

Edit: Ah, I forgot that git reset --hard <rev> should be possible.

pedropombeiro · 2018-02-14T08:53:18Z

_assets/patches/patcher

+			git apply "$f" --directory="$basepath" > /dev/null 2>&1
+		done
+		# Sorts out new patches' paths by comparing with "develop" branch.
+		fromDevelop=($(git diff develop --stat | grep "\\.patch" |


What do you think about making develop a baseBranch variable at the beginning of the script next to basepath and others and rename fromDevelop to fromBaseBranch? That would make it easy to see all assumptions in one place at the beginning of the script and promote reuse in future script additions.

Good idea! Thanks.

pedropombeiro · 2018-02-14T08:57:14Z

geth/node/node.go

@@ -182,15 +181,6 @@ func activateShhService(stack *node.Node, config *params.NodeConfig) error {
 			mailServer.Init(whisperService, whisperConfig.DataDir, whisperConfig.Password, whisperConfig.MinimumPoW)
 		}

-		// enable notification service


@mandrigin Is this code not needed anymore?

We actually should move it to the notifications patch, I think.
It isn't really dependent on anything in status-go.

@adambabik @divan @JekaMas what do you think?

@canercidam can you please move that removed code into 0004-whisper-notifications.patch and just expose EnableNotifications() method from whisperService?
That's the last thing that is left in this PR, and we can merge it!

(PS: Looking at that it takes more time than we expected, we made it a bigger M bounty).

pedropombeiro · 2018-02-14T09:08:40Z

Makefile

+	./_assets/patches/patcher -c
+	./_assets/ci/isolate-vendor-check.sh
+
+dep-ensure:


Any reason why we're not including these targets in the help screen of Make so people are aware of them when they type make? We could have patches and dependencies sections. What do you think @mandrigin?

I think we should definitely include them

canercidam · 2018-02-14T12:31:27Z

@mandrigin
what would happen to vendor-check if the patch was changed but the changes didn't apply?

Complains:

Recently added but not applied: /home/mo/gopath/src/github.com/status-im/status-go/_assets/patches/geth/0009-whisper-envelopes-tracing.patch

pedropombeiro · 2018-02-14T15:19:49Z

_assets/patches/patcher

@@ -56,7 +60,7 @@ while getopts :prcv opt; do
 			git apply "$f" --directory="$basepath" > /dev/null 2>&1
 		done
 		# Sorts out new patches' paths by comparing with "develop" branch.


replace "develop" in comment

pedropombeiro

Just a tiny change requested, but other than that it is approved

pedropombeiro · 2018-02-14T15:45:47Z

@canercidam thanks. So now we just need to include the removed geth/node/node.go code in the 0004-whisper-notifications.patch patch and it seems like we're done!

mandrigin

I think we should make EnableNotifications() method in whisperService and make it a part of a patch (0004...).
Otherwise, we will have the same problems with dep being unable to find a correct go-ethereum branch when upgrading this dependency.

canercidam · 2018-02-14T16:23:21Z

@mandrigin Patching status-go code?

Before doing that, I'm wondering if it's really necessary to have whisper/notifications in vendored go-ethereum instead of status-go.

mandrigin · 2018-02-14T16:39:33Z

@canercidam this package shouldn't be in go-ethereum, I think.
We have an issue (#655) to deal with that.

Though it would be a huge scope creep to do that in this PR, I think. I'd like to have it merged :)
So I suggest a workaround that still keeps notifications on and removes a dependency from go-ethereum/whisper/notifications.

Change the code in status-go code to

		if whisperConfig.EnablePushNotification {
 			log.Info("Register PushNotification server")
                        whisperService.EnableNotificationServer()
 		}

Move the declaration/implementation of this method to go-ethereum (and 0004- patch).

canercidam · 2018-02-14T16:49:17Z

@mandrigin That sounds nice but wouldn't that cause an import cycle problem? whisper/notifications use whisperv5 and whisperv5 will have a method for whisper/notifications.

mandrigin · 2018-02-14T16:58:39Z

@canercidam you are right.
Ok, let's just use https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md#ignored then so dep don't scream at us when upgrading dependency.

GitHub
golang/dep
Go dependency management tool

canercidam · 2018-02-14T17:31:24Z

Thank you, added. I think that was a clean solution. 👍

mandrigin

Looks great! Thanks a lot, amazing job, @canercidam!

canercidam · 2018-02-14T17:47:05Z

🎉

canercidam added 7 commits February 13, 2018 15:55

add -c option to patcher and new targets

dfef258

check uncommitted as well

62f6eb1

quiet

5c4b33d

remove whisper/notifications dependency and use ethereum/go-ethereum

0fbbe46

fix comment

161877a

dep ensure update

8209112

make dep-ensure

e3e6d0e

canercidam mentioned this pull request Feb 13, 2018

Apply geth patches on the fly. #653

Closed

fix vendor

9c9a035

themue added the in progress label Feb 13, 2018

fix vendor-check

337f0a6

canercidam force-pushed the issue-653 branch from 4033589 to 337f0a6 Compare February 13, 2018 18:32

mandrigin requested review from adambabik, pedropombeiro and dshulyak February 14, 2018 08:19

mandrigin suggested changes Feb 14, 2018

View reviewed changes

pedropombeiro suggested changes Feb 14, 2018

View reviewed changes

add new targets to make help screen

76f2cec

canercidam added 5 commits February 14, 2018 15:41

avoid confusion

1224db1

define base branch in patcher

3dce222

keep quiet about git stash apply

5df0550

add timestamp to temp branch

1a3ef56

update docs

33106d6

canercidam changed the title ~~WIP Apply geth patches on the fly #653~~ Apply geth patches on the fly #653 Feb 14, 2018

pedropombeiro reviewed Feb 14, 2018

View reviewed changes

pedropombeiro approved these changes Feb 14, 2018

View reviewed changes

fix comment

c60879f

pedropombeiro mentioned this pull request Feb 14, 2018

Investigate/perform upgrade to Geth 1.8.1 #665

Closed

6 tasks

add removed code in geth/node/node.go

e829adb

mandrigin suggested changes Feb 14, 2018

View reviewed changes

ignore whisper/notifications in Gopkg.toml

db61cd3

mandrigin approved these changes Feb 14, 2018

View reviewed changes

mandrigin merged commit 15c72f3 into status-im:develop Feb 14, 2018


		branchName="$(git rev-parse --abbrev-ref HEAD)"

		git checkout -b isolated-vendor-check

Apply geth patches on the fly #653 #660

Apply geth patches on the fly #653 #660

Conversation

canercidam commented Feb 13, 2018 • edited Loading

mandrigin commented Feb 13, 2018

canercidam commented Feb 13, 2018

mandrigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedropombeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canercidam Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canercidam commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedropombeiro left a comment

Choose a reason for hiding this comment

pedropombeiro commented Feb 14, 2018

mandrigin left a comment

Choose a reason for hiding this comment

canercidam commented Feb 14, 2018

mandrigin commented Feb 14, 2018

canercidam commented Feb 14, 2018

mandrigin commented Feb 14, 2018 • edited by ghost Loading

canercidam commented Feb 14, 2018

mandrigin left a comment

Choose a reason for hiding this comment

canercidam commented Feb 14, 2018

canercidam commented Feb 13, 2018 •

edited

Loading

canercidam Feb 14, 2018 •

edited

Loading

mandrigin commented Feb 14, 2018 •

edited by ghost

Loading