Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Make executable names configurable #7118

Merged
merged 3 commits into from
Apr 18, 2019

Conversation

conr2d
Copy link
Contributor

@conr2d conr2d commented Apr 12, 2019

Change Description

CMakeLists.txt has options for changing the name of executables like nodeos, keosd or cleos, but there are hard-coded strings in their source code. This PR makes executable names configurable by setting path to changed name or applying new name in help text.

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

@@ -48,7 +48,7 @@ int main(int argc, char** argv)
if(!app().initialize<wallet_plugin, wallet_api_plugin, http_plugin>(argc, argv))
return -1;
auto& http = app().get_plugin<http_plugin>();
http.add_handler("/v1/keosd/stop", [](string, string, url_response_callback cb) { cb(200, fc::variant(fc::variant_object())); std::raise(SIGTERM); } );
http.add_handler("/v1/" + keosd::config::key_store_executable_name + "/stop", [](string, string, url_response_callback cb) { cb(200, fc::variant(fc::variant_object())); std::raise(SIGTERM); } );
Copy link
Contributor

Choose a reason for hiding this comment

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

need to change the corresponding line in httpc.hpp too

@spoonincode spoonincode dismissed their stale review April 12, 2019 19:36

removing my block

@@ -1557,7 +1558,7 @@ launcher_def::launch (eosd_def &instance, string &gts) {
install_path = specific_nodeos_installation_paths[node_num] + "/";
}
}
string eosdcmd = install_path + "programs/nodeos/nodeos ";
string eosdcmd = install_path + "programs/nodeos/" + string(node_executable_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

you lost a trailing space here, and it might be why tests are failing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I applied it.

("skip-signature", bpo::bool_switch(&skip_transaction_signatures)->default_value(false), (string(node_executable_name) + " does not require transaction signatures.").c_str())
(node_executable_name, bpo::value<string>(&eosd_extra_args), ("forward " + string(node_executable_name) + " command line argument(s) to each instance of " + string(node_executable_name) + ", enclose arg(s) in quotes").c_str())
("specific-num", bpo::value<vector<uint>>()->composing(), ("forward " + string(node_executable_name) + " command line argument(s) (using \"--specific-" + string(node_executable_name) + "\" flag) to this specific instance of " + string(node_executable_name) + ". This parameter can be entered multiple times and requires a paired \"--specific-" + string(node_executable_name) +"\" flag each time it is used").c_str())
(("specific-" + string(node_executable_name)).c_str(), bpo::value<vector<string>>()->composing(), ("forward " + string(node_executable_name) + " command line argument(s) to its paired specific instance of " + string(node_executable_name) + "(using \"--specific-num\"), enclose arg(s) in quotes").c_str())
Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately changing these config names will mean that the python test scripts will not operate when node_executable_name is not 'nodeos'

maybe these two config items should be renamed to something else that doesn't include "nodeos"? (to avoid having to preprocess all the various python scripts with whatever is configured)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found the scripts you mentioned, and I guess there can be another cases which contain hard-coded these options. I don't like this way much, but what about adding new option like -node-exec, but also leaving -nodeos for backward compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

come to think of it, those scripts won't work before your change anyways because of hardcoded "nodeos", "keosd", etc in the scripts too. Would be nice to take care of that in the future, but I think PR is ready as-is

@spoonincode spoonincode merged commit 9fc484a into EOSIO:develop Apr 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants