Better command line argument handling #3628 by demon1999 · Pull Request #4597 · ethereum/aleth

demon1999 · 2017-10-12T18:54:42Z

Resolves #3628

gumb0 · 2017-10-13T17:18:39Z

Thanks a lot for your work! I'll try to look at it next week.

You could rebase it on the latest develop and resolve conflicts meanwhile.

pirapira · 2017-10-18T09:19:05Z

@demon1999 are you still around?

demon1999 · 2017-10-18T09:23:06Z

Yes, I am. Now I'm reading about rebase.

chfast · 2017-10-18T10:43:23Z

@demon1999 it would be better to split this PR into smaller parts. Can you make one that adds only boost program_options first. Then a PR that converts options for one of the smaller tools like rlp or ethvm.

winsvega · 2017-10-18T11:29:27Z

right. it is better to be done module by module. Id like to test testeth options first.

demon1999 · 2017-10-18T12:44:29Z

Ок, I'll do it when I'll have time.

chfast · 2017-10-19T14:00:38Z

Hi again.
Please also see #4613.
They are proposed changes to ethvm command line interface, so we might want to do them the same time.

gumb0 · 2017-10-23T12:14:38Z

eth/AccountManager.cpp

 #include <libdevcore/FileSystem.h>
 #include <libdevcore/CommonIO.h>
 #include <libethcore/KeyManager.h>
+#include <boost/program_options.hpp>


Are these includes needed? Only includes are added to this file and nothing else.

I think, I don't need it.

gumb0 · 2017-10-23T12:16:14Z

eth/main.cpp

+            ("private", po::value<string>(), "<name> Use a private chain.")
+            ("test", "Testing mode: Disable PoW and provide test rpc interface.")
+            ("config", po::value<string>(), "<file> Configure specialised blockchain using given JSON information.")
+            ("oppose-dao-fork", "Ignore DAO hard fork (default is to participate).\n")


This can be deleted

This and -f options are not supported anymore.
Probably there're more outdated options, I'll take a closer look later

gumb0 · 2017-10-23T12:16:39Z

eth/main.cpp

+			("address,a", po::value<string>(), "<addr>  Set the author (mining payout) address to given address (default: auto).")
+			("author", po::value<string>(), "<addr>  Set the author (mining payout) address to given address (default: auto).")
+			("mining,m", po::value<string>(), "<on/off/number>  Enable mining, optionally for a specified number of blocks (default: off).")
+			("force-mining,f", "Mine even when there are no transactions to mine (default: off).")


This can be deleted

all this 4 options?

Only -f & --force-mining
mining itself is still supported

gumb0 · 2017-10-23T12:18:43Z

eth/main.cpp

+    clientDefaultMode.add_options()
+		    ("format", po::value<string>(), "<format> Set format.")
+		    ("script", po::value<string>(), "<script> Add script.")
+            ("mainnet", "Use the main network protocol.")


Some lines are indented with spaces, please change to tabs everywhere

gumb0 · 2017-10-27T17:07:23Z

CMakeLists.txt

+
+add_executable(main rlp/main.cpp)
+
+add_executable(peer test/unittests/libp2p/peer.cpp)


Why do we need this?

gumb0 · 2017-10-27T17:09:23Z

ethkey/main.cpp

+		("verbosity,v", po::value<string>(), "<0 - 9>  Set the log verbosity from 0 to 9 (default: 8).")
+		("version,V", "Show the version and exit.")
+		("help,h",  "Show this help message and exit.")
+	;


This formatting looks weird in our style, please leave semicolon on the last line of the statement.

gumb0 · 2017-10-27T17:11:14Z

ethkey/main.cpp

-
+	po::options_description generalOptions("General Options");
+	generalOptions.add_options()
+		("verbosity,v", po::value<string>(), "<0 - 9>  Set the log verbosity from 0 to 9 (default: 8).")


Couldn't we make it po::value<int> ?

gumb0 · 2017-10-27T17:12:40Z

ethkey/main.cpp


+namespace po = boost::program_options;
+
 void help()


Do we still need this function?

gumb0 · 2017-10-27T17:18:43Z

ethkey/main.cpp

-			help();
-		else if (arg == "-V" || arg == "--version")
-			version();
+		else if (((arg == "-v" || arg == "--verbosity") && i + 1 < argc) || arg == "-h" || arg == "--help" || arg == "-V" || arg == "--version") {


opening brace should be on a separate line

gumb0 · 2017-10-27T17:31:01Z

ethkey/main.cpp

-		else if (arg == "-V" || arg == "--version")
-			version();
+		else if (((arg == "-v" || arg == "--verbosity") && i + 1 < argc) || arg == "-h" || arg == "--help" || arg == "-V" || arg == "--version") {
+			argv[ac] = argv[i];


Could you explain what are you trying to achieve in this part?
I don't think assiging to argv is a very good idea

I think you're trying to get rid of all the options that are handled in KeyCLI::interpretOption() and leave only -v, -V, -h in argv
Why do you need this? Wouldn't po::store(po::parse_command_line(ac, argv, generalOptions), vm); below just ignore options not listed in generalOptions ?

Also look at this http://www.boost.org/doc/libs/1_65_1/doc/html/program_options/howto.html#idp437834784

demon1999 · 2017-10-28T15:06:54Z

#4635

gumb0 · 2017-10-30T19:12:28Z

@demon1999 Are you still planning to update this PR or should we close this one and you will proceed with several smaller PRs?

demon1999 · 2017-10-30T19:15:00Z

You should close it. One of small pr's is 4635

demon1999 added 12 commits September 25, 2017 23:01

Tryed to add boost::program_options library

d7721df

Adding a boost::program_options library

e5917bd

Adding client mode(default) options

a4cfdec

Adding client transacting options and client mining options

f20ff93

fixing todo

5f23851

adding clientNetworking options

0febf63

Rewriting command string arguments in ethvm and eth

eb4f786

Finish of rewriting ethvm

eb1cd28

Fixing ethvm/main.cpp and ethkey/main.cpp

7ce58ad

Fixing eth/main.cpp and rip.cpp and peer.cpp

af0d89a

Fixing eth/main.cpp

691d0e6

Fixing test/tools/libtesteth/Options.cpp

bf58fff

gumb0 requested review from chfast and gumb0 October 13, 2017 17:18

gumb0 added the needs review label Oct 13, 2017

gumb0 suggested changes Oct 23, 2017

View reviewed changes

demon1999 added 2 commits October 24, 2017 08:26

Fixing codestyle

c1f13cb

Deliting useless options

5612846

gumb0 mentioned this pull request Oct 24, 2017

Download snapshot through Parity's warp protocol #4622

Merged

5 tasks

gumb0 suggested changes Oct 27, 2017

View reviewed changes

gumb0 closed this Oct 30, 2017


		add_executable(main rlp/main.cpp)

		add_executable(peer test/unittests/libp2p/peer.cpp)

Conversation

demon1999 commented Oct 12, 2017 • edited by gumb0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gumb0 commented Oct 13, 2017

Uh oh!

pirapira commented Oct 18, 2017

Uh oh!

demon1999 commented Oct 18, 2017

Uh oh!

chfast commented Oct 18, 2017

Uh oh!

winsvega commented Oct 18, 2017

Uh oh!

demon1999 commented Oct 18, 2017

Uh oh!

chfast commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

demon1999 commented Oct 28, 2017

Uh oh!

gumb0 commented Oct 30, 2017

Uh oh!

demon1999 commented Oct 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

demon1999 commented Oct 12, 2017 •

edited by gumb0

Loading