These are chat archives for ethersphere/orange-lounge

25th
Sep 2017
Viktor Trón
@zelig
Sep 25 2017 08:58
ok @nolash have you rebased on the branch of #120
lash
@nolash
Sep 25 2017 09:00
@zelig no
Lewis Marshall
@lmars
Sep 25 2017 09:07
@zelig I think we need to consolidate these branches before moving forward
the branch of 120 has diverged from network-testing-framework which other people have been working from
p2p-simulations is now merged :)
Viktor Trón
@zelig
Sep 25 2017 09:09
yes work needs to be rebased on the new network rewrite
and network rewrite rebased on master
could you do it?
Lewis Marshall
@lmars
Sep 25 2017 09:09
so we want to drop the network-testing-framework branch?
Viktor Trón
@zelig
Sep 25 2017 09:18
i think so
Lewis Marshall
@lmars
Sep 25 2017 09:19
ok, so should we perhaps rename the rewrite to something like swarm-network-rewrite?
Viktor Trón
@zelig
Sep 25 2017 09:19
yes
Lewis Marshall
@lmars
Sep 25 2017 09:19
and then have a pss branch on top of that with all the pss work?
Lewis Marshall
@lmars
Sep 25 2017 09:34
@zelig what do you want to do about this PR? ethersphere/go-ethereum#119
perhaps just PR it to master now?
Viktor Trón
@zelig
Sep 25 2017 09:35
did you not integrate those commits in the end?
I thought you said you would
Lewis Marshall
@lmars
Sep 25 2017 09:36
I attempted to add DialPeer but it proved difficult, and then I did not get round to the other things, and in the meantime Felix has merged the PR
so we should just do another PR
lash
@nolash
Sep 25 2017 09:44
Maybe I should hold off rebasing until you agree? :)
Lewis Marshall
@lmars
Sep 25 2017 09:44
yep, I'll let you know when I've moved things around
lash
@nolash
Sep 25 2017 09:44
ty @lmars
Lewis Marshall
@lmars
Sep 25 2017 10:36
@zelig see ethereum/go-ethereum#15198
we need to decide what to do about this hack
Lewis Marshall
@lmars
Sep 25 2017 10:44

@zelig @lash I have created swarm-network-rewrite which is now on top of p2p-simulations-fixes (and so also on top of master).

The code does fail to compile now though:

$ go test -v ./swarm
# github.com/ethereum/go-ethereum/swarm/api
swarm/api/config.go:48: undefined: network.SyncParams

and also TestKademliaHiveString fails for me.

the plan is to have the following chain of branches:

  • p2p-simulations-fixes - currently PR'd as ethereum/go-ethereum#15198
  • swarm-network-rewrite - the network rewrite including pot, kademlia, hive, dicovery_test etc.
  • pss - all the pss related changes

Then PR in that order, sounds good?

any changes should be PR'd into those respective branches and then we should keep everything rebased after merging anything
lash
@nolash
Sep 25 2017 10:58
That sounds good to me
I will proceed and rebase on swarm-network-rewrite ten
s/ten/then/
Lewis Marshall
@lmars
Sep 25 2017 10:58
:+1:
lash
@nolash
Sep 25 2017 10:59
holy fuck
179 commits!!
in the rebase
Lewis Marshall
@lmars
Sep 25 2017 10:59
yep so it's messy
you'll probably stuggle to actually do a rebase
lash
@nolash
Sep 25 2017 11:00
Can't I just wipe out the pss dir and overwrite?
I mean, I'm the only one doing changes there anyway
Lewis Marshall
@lmars
Sep 25 2017 11:00
instead just checkout swarm-network-rewrite and pull in your pss changes, create a single commit
lash
@nolash
Sep 25 2017 11:01
by "pull in" you mean overwrite the dir, right?
Lewis Marshall
@lmars
Sep 25 2017 11:01
but there are some pss related changes which aren't in the swarm-network-rewrite branch and also not in swarm/pss
well, like git checkout COMMIT swarm/pss will just checkout swarm/pss with all the changes from COMMIT
but there are also some other files you've changed (e.g. cmd/swarm/main.go)
lash
@nolash
Sep 25 2017 11:02
I can rewrite the cmd file
It's probably faster
Lewis Marshall
@lmars
Sep 25 2017 11:02
:+1:
yep
lash
@nolash
Sep 25 2017 11:03
If I checkout the latest commit of pss-whisper-noaddr onto swarm-network-rewrite, will it apply the FULL diff between those files?
gith
tright
it will just overwrite righT?
never mind
I'm writing before I think
lash
@nolash
Sep 25 2017 12:47
```WARN [09-25|14:42:20] start up failed: key not found: peers caller=network.go:193
```
do we know what this is?
lash
@nolash
Sep 25 2017 19:46
@lmars I PR to master or swarm-network-rewrite?
Lewis Marshall
@lmars
Sep 25 2017 19:56

so the aim is to manage a pss branch which we will eventually merge into master after all the other work.

I was imagining we would push all the changes which have already been reviewed to a pss branch (on top of swarm-network-rewrite) and then any pss related changes which haven't yet been reviewed would be PRd into pss.

That way we keep a pss branch which just has pss related changes which will be ready to PR as soon as swarm-network-rewrite is merged into master.

Does that make sense?

lash
@nolash
Sep 25 2017 20:00
theoretically yes, but
I moved around a lot of the code in the latest edits, so I'm not sure how much better of an overview diffs to the last review state would be than just reviewing directly onto master.
but if you think this is best, I guess I would reset the branch in the old PR to ... I dunno, the next commit after the review, then rebase that on swarm-network-rewrite, create the pss branch from that, then PR my new branch that I prepared now to that?
Lewis Marshall
@lmars
Sep 25 2017 20:04
ok perhaps just bundle all the pss changes so far and we can just review the whole thing, even if it contains already reviewed stuff
lash
@nolash
Sep 25 2017 20:04
I've amended all the last review comments anyway
If we do it like this, then it's all set to go already in the pss branch I just uploaded
s/uploaded/pushed/
So I PR this to what?
Lewis Marshall
@lmars
Sep 25 2017 20:06
ok cool so ethersphere/pss contains all the pss changes now?
lash
@nolash
Sep 25 2017 20:06
yes, and it's based on swarm-network-rewrite
Lewis Marshall
@lmars
Sep 25 2017 20:07

so if you want it all reviewed then I suggest pushing pss back to swarm-network-rewrite, creating a branch pss-review at the current commit, then pull requesting pss-review into pss.

That way, once merged, we'll end up with a reviewed pss branch on top of swarm-network-rewrite.

What do you think?

lash
@nolash
Sep 25 2017 20:07
that's fine!
lash
@nolash
Sep 25 2017 20:33
@lmars @zelig ethersphere/go-ethereum#123
Lewis Marshall
@lmars
Sep 25 2017 21:05
nice, I'll review it myself when I get chance
lash
@nolash
Sep 25 2017 21:06
appreciate it @lmars
holisticode
@holisticode
Sep 25 2017 21:30
@lmars PR #122 updated
Lewis Marshall
@lmars
Sep 25 2017 22:13
@holisticode I reviewed it, we should probably pull request everything but the change to overlay directly to master once this is merged
holisticode
@holisticode
Sep 25 2017 23:27
why not the change to overlay?
Lewis Marshall
@lmars
Sep 25 2017 23:27
because the overlay isn't on master, but the p2p/simulations stuff is
holisticode
@holisticode
Sep 25 2017 23:28
won't it be at some point?
Lewis Marshall
@lmars
Sep 25 2017 23:28
sure, I just meant you may as well PR the p2p/sim changes directly to master, less code for us to keep on these branches
we really need to start merging this stuff :)
holisticode
@holisticode
Sep 25 2017 23:29
I surely agree
so you suggest to leave this PR alone, wait until p2p-simulations is merged, and redo the PR onto that?
Lewis Marshall
@lmars
Sep 25 2017 23:31
p2p-simulations is merged already, that's my point :wink:
holisticode
@holisticode
Sep 25 2017 23:31
uh
Lewis Marshall
@lmars
Sep 25 2017 23:31
ethereum/go-ethereum#14982
holisticode
@holisticode
Sep 25 2017 23:33
so redo #122 onto master
but what about overlay
I need to point the frontend to a consistent branch...
I mean overlay was meant as the backend for the frontend
if I PR #122 onto master - I could include overlay in that PR?
holisticode
@holisticode
Sep 25 2017 23:43
@holisticode I reviewed it, we should probably pull request everything but the change to overlay directly to master once this is merged
"once this is merged" though also could suggest that we keep and merge #122 and from then on work from master