Chain-selection from UI #4859

gavofyork · 2017-03-10T16:04:20Z

No description provided.

arkpar · 2017-03-10T16:19:26Z

Looks good for now. However hypervisor approach will make it hard to implement changing settings such as chain, ports, etc. in parity-as-a library setting. We'd probably have to reimplement this so it does not require process shutdown eventually. This would require careful accounting for any shutdown leaks. AFAIK some of the services currently (IPC/RPC/stratum/whatever) don't cleanup 100% after themselves and may leave a thread hanging or some memory stuck in a pointer loop. This will require quite a bit of work and maybe automated tests that would check that there are no memory/thread/FD leaks.

jacogr · 2017-03-12T08:27:29Z

js/src/views/Settings/Parity/parity.spec.js

@@ -94,5 +100,26 @@ describe('views/Settings/Parity', () => {
        expect(instance.store.changeMode).to.have.been.calledWith('dark');
      });
    });
+    describe('chain selector', () => {


Blank line before.

jacogr · 2017-03-12T08:27:44Z

js/src/views/Settings/Parity/store.spec.js

  describe('@action', () => {
    describe('setMode', () => {
      it('sets the mode', () => {
        store.setMode('offline');
        expect(store.mode).to.equal('offline');
      });
    });
+    describe('setChain', () => {


Blank line before.

jacogr · 2017-03-12T08:28:55Z

2 tiny comments, otherwise code looks good.

[ci:skip]

arkpar · 2017-03-12T11:08:43Z

ethcore/src/client/client.rs

+	}
+
+	/// Hypervisor should kill this client.
+	pub fn exit(&self) {


Where is this called from? Why not just implement Drop?

exit and restart are not yet called from anywhere, however, they seem like useful public api to have; i plan on implementing a UI endpoints for them to all the user to kill or restart parity easily.

I don't see how it belongs in Client, whose role is just to manage and provide blockchain data. Hypervisor stuff being at a higher level would make it a lot simpler to manage different kinds of clients, parity-as-a-library, non-hypervised instances, etc.

will remove for the present PR.

i guess we might want to look into an alternative "hypervisor" IPC endpoint that could host such calls, however in the context of the present PR that would be too much extra baggage.

arkpar · 2017-03-12T11:18:58Z

parity/main.rs

-				PostExecutionAction::Restart => PLEASE_RESTART_EXIT_CODE,
+				PostExecutionAction::Restart(spec_name_override) => {
+					if let Some(spec_name) = spec_name_override {
+						set_spec_name_override(spec_name);


What about all other CLI options? Are they not preserved?

yes, but that happens as part of the standard hypervisor process. this is specifically handling the instance where the user has altered the chain spec via the UI.

arkpar · 2017-03-12T11:22:06Z

parity/main.rs

@@ -208,6 +208,22 @@ fn latest_exe_path() -> Option<PathBuf> {
 		.and_then(|mut f| { let mut exe = String::new(); f.read_to_string(&mut exe).ok().map(|_| updates_path(&exe)) })
 }

+fn set_spec_name_override(spec_name: String) {
+	if let Err(e) = File::create(updates_path("spec_name_overide"))


Why is there a need for a file at all? Can't the hypervisor just pass CLI params to a new process?

Why not introduce --chain=auto as default setting and just update user_defaults like we do with pruning, tracing and fat_db?

Why is there a need for a file at all? Can't the hypervisor just pass CLI params to a new process?

two reasons:

getting data out a child process back to the parent cleanly is non-trivial. i used a return-code thus far but it's no good for information-rich content. i guess we could use std-out but then it wouldn't work properly in all circumstances.

this method works well for existing auto-update installations; using the hypervisor to relay information (even if it were easy) would mean everyone manually re-installing before this feature worked, since the hypervisor process is always the most recent manually installed executable.

Why not introduce --chain=auto as default setting and just update user_defaults like we do with pruning, tracing and fat_db?

(I actually implemented it this way originally.) user_defaults is per-chain, which obv. won't work for the setting of which chain to use. we could use another non-per-chain user-defaults, but then that's a lot of extra baggage for what is really just temporary signalling.

…hain-select

First little bits for chain-selection.

49ee31a

gavofyork added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. M7-ui labels Mar 10, 2017

gavofyork added this to the 1.7 milestone Mar 10, 2017

gavofyork added 7 commits March 10, 2017 18:37

Provide RPCs and get settings through to user defaults.

bc7ca7e

Hasty stash.

de821eb

Fix updater accidentally redownloading.

eb09caa

Finish up.

477c173

Add JS tests.

4c81f8b

Hypervisor should never run a binary modified before itself.

ec5987b

Style.

379bb06

gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 11, 2017

gavofyork added 3 commits March 11, 2017 13:28

Help tweak.

6a1209c

Fix test compile.

bfdbecb

Merge remote-tracking branch 'origin/master' into ui-chain-select

3793c90

gavofyork changed the title ~~First little bits for chain-selection.~~ Chain-selection from UI Mar 11, 2017

gavofyork added 6 commits March 11, 2017 16:28

Fix JS test

2608f80

Build fix for tests.

08f4b5e

Revert default chain name

81021c9

Another test

94f26a1

Use spec name via client.

1d1c055

Fix mock up.

b07dc72

jacogr reviewed Mar 12, 2017

View reviewed changes

whitespace

cdc6a39

[ci:skip]

whitespace

c3c0d4f

[ci:skip]

arkpar reviewed Mar 12, 2017

View reviewed changes

arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 12, 2017

gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Mar 12, 2017

gavofyork added 3 commits March 13, 2017 11:38

remove exit/restart endpoints.

c8441bc

Merge branch 'ui-chain-select' of github.com:ethcore/parity into ui-c…

c0e2061

…hain-select

Merge remote-tracking branch 'origin/master' into ui-chain-select

338cf86

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 13, 2017

gavofyork merged commit 3041c95 into master Mar 13, 2017

gavofyork deleted the ui-chain-select branch March 13, 2017 11:10

gavofyork added the B0-patch label Mar 13, 2017

ngotchac mentioned this pull request Mar 13, 2017

UI network switcher #4682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chain-selection from UI #4859

Chain-selection from UI #4859

gavofyork commented Mar 10, 2017

arkpar commented Mar 10, 2017

jacogr Mar 12, 2017

jacogr Mar 12, 2017

jacogr commented Mar 12, 2017

arkpar Mar 12, 2017

gavofyork Mar 12, 2017

rphmeier Mar 12, 2017

gavofyork Mar 13, 2017

gavofyork Mar 13, 2017

arkpar Mar 12, 2017

gavofyork Mar 12, 2017 •

edited

Loading

arkpar Mar 12, 2017

tomusdrw Mar 12, 2017

gavofyork Mar 12, 2017

Chain-selection from UI #4859

Chain-selection from UI #4859

Conversation

gavofyork commented Mar 10, 2017

arkpar commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr commented Mar 12, 2017

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

gavofyork Mar 12, 2017 • 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

gavofyork Mar 12, 2017 •

edited

Loading