network/simulation: Add ExecAdapter capability to swarm simulations #1503
Conversation
nolash
added
ready for review
ready for another review
and removed
ready for review
labels
|
@nolash approved, provided tests are passing, I can see a few more references to |
network/simulation/simulation.go
Outdated
| } | ||
|
|
||
| adapterServices := s.addServices(services) |
There was a problem hiding this comment.
This is still confusing (but better than before), because s.addServices is not only returning adapterServices, but also modifying s.
Maybe it'd be better is line 111 was part of s.addServices so that you don't have to return the adapterServices and rely on s.addServices to add them to the simulation and register them.
There was a problem hiding this comment.
Thing is we don't need to RegisterServices there for the inproc node.
eh - I didn't mean to do that. |
|
@nolash yeah, this is configured by default now - if someone approves a PR and you add a commit, the reviews are dismissed. I also forgot about that. |
| adapterServices := s.toAdapterServices(services) | ||
|
|
||
| // exec adapters register services up front, not at node creation time | ||
| adapters.RegisterServices(adapterServices) |
There was a problem hiding this comment.
@nolash We have a problem with this RegisterServices() function. It should be called within an init() function as described in the documentation: https://github.com/ethereum/go-ethereum/tree/master/p2p/simulations#services
So basically, to make the current exec/docker adapter work. we would have to register all services upfront.
In our case it gets a bit harder, because we attached this to (s *Simulation)... so unless we define all our simulations inside an init(), this won't work.
I've been thinking now about this for a while and I don't find a decent solution.... any idea?
fyi @nonsense
There was a problem hiding this comment.
I had completely forgotten about this requirement, I remember reading about it a year or so ago...
I also don't have any solutions :(
There was a problem hiding this comment.
In our case it gets a bit harder
Why exactly do we need to put it in an init (except for the doc claiming that we do)?
* master: network/newstream: new stream! protocol base implementation (#1500) swarm: fix bzz_info.port when using dynamic port allocation (#1537) cmd/swarm: make bzzaccount flag optional and add bzzkeyhex flag (#1531) cmd/swarm: remove separate function to parse env vars (#1536) network/bitvector: Multibit set/unset + string rep (#1530) swarm: 0.4.3 unstable (#1526) travis: also build on release tags (#1527) swarm: release v0.4.2 (#1496) network: bump bzz stream hive (#1522) docker: update ca-certificates file (#1525) Add swarm guide to /docs (#1513) network/simulation: Add ExecAdapter capability to swarm simulations (#1503)
This change makes it possible to select
ExecAdapterfor the Swarm simulations wrapper package.