Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use executables -help menu instead of man page to get options #34

Merged
merged 1 commit into from
Dec 11, 2021
Merged

use executables -help menu instead of man page to get options #34

merged 1 commit into from
Dec 11, 2021

Conversation

AbcSxyZ
Copy link
Contributor

@AbcSxyZ AbcSxyZ commented Dec 10, 2021

Fix #28. Needed also for test coverage of Dockerfile in #25.

Use of -help menu to get option instead of man pages.

Do you think the regex from grep is safe : ^ -' ? It handles all options with 2 space from the start of the line, do you know if it's done like that ? I started with ^ *-, but some options may use others in their description like -listen or -checkblockindex.

What would you like to hook from here ? Do we stay with grep, or we could break the grep and use regex from python to generate a more raw entry easier to hook ? The grep option may be nicer but harder to test.

With the grep, we may hook with something like this:

dogecoind_menu = [
    "  -blockmintxfee=<amt>",
    "  -datadir=<dir>",
    "  -server",
    ...
]
dogecoin_cli_menu = [
    "  -datadir=<dir>",
    ...
]

Or if we want to test also the regex on a raw file, we break it and return some raw string like the help menu.

Because it uses executables, a test from #25 is breaking for dogecoin-qt with the following:

E               subprocess.CalledProcessError: Command 'dogecoin-qt -help -help-debug | grep '^  -'' returned non-zero exit status 1.

/usr/lib/python3.9/subprocess.py:528: CalledProcessError
------------------------------------- Captured stderr call --------------------------------------
dogecoin-qt: error while loading shared libraries: libxkbcommon-x11.so.0: cannot open shared object file: No such file or directory

Issue related to X window system, would be great to have a discussion related to dogecoin-qt as you mentioned previously, to see how/if we handle it.

@AbcSxyZ AbcSxyZ changed the title use excutable -help instead of man page to get options use executables -help menu instead of man page to get options Dec 10, 2021
@patricklodder patricklodder added the enhancement New feature or request label Dec 11, 2021
@patricklodder
Copy link
Member

Do you think the regex from grep is safe : ^ -'?

The only thing I don't like is the inclusion of -? because that won't easily work as an environment variable.

I tried grep '^ -[-a-z0-9]\+\(=.*\)\?$', on both 1.14 and 1.21:

$ ./dogecoind-1.14 -help -help-debug |grep '^  -[-a-z0-9]\+\(=.*\)\?$' |sort
  -acceptnonstdtxn
  -addnode=<ip>
  -alertnotify=<cmd>
  -alerts
  -assumevalid=<hex>
  -banscore=<n>
  -bantime=<n>
  -bind=<addr>
  -bip9params=deployment:start:end
  -blockmaxsize=<n>
  -blockmaxweight=<n>
  -blockmintxfee=<amt>
  -blocknotify=<cmd>
  -blockprioritysize=<n>
  -blockreconstructionextratxn=<n>
  -blocksonly
  -blockversion=<n>
  -bytespersigop
  -checkblockindex
  -checkblocks=<n>
  -checklevel=<n>
  -checkmempool=<n>
  -checkpoints
  -conf=<file>
  -connect=<ip>
  -daemon
  -datacarrier
  -datacarriersize
  -datadir=<dir>
  -dbcache=<n>
  -dblogsize=<n>
  -debug=<category>
  -disablesafemode
  -disablewallet
  -discardthreshold=<amt>
  -discover
  -dns
  -dnsseed
  -dropmessagestest=<n>
  -dustlimit=<amt>
  -externalip=<ip>
  -fallbackfee=<amt>
  -feefilter
  -flushwallet
  -forcednsseed
  -fuzzmessagestest=<n>
  -harddustlimit=<amt>
  -help-debug
  -incrementalrelayfee=<amt>
  -keypool=<n>
  -limitancestorcount=<n>
  -limitancestorsize=<n>
  -limitdescendantcount=<n>
  -limitdescendantsize=<n>
  -limitfreerelay=<n>
  -listen
  -listenonion
  -loadblock=<file>
  -logips
  -logtimemicros
  -logtimestamps
  -maxconnections=<n>
  -maxmempool=<n>
  -maxorphantx=<n>
  -maxreceivebuffer=<n>
  -maxsendbuffer=<n>
  -maxsigcachesize=<n>
  -maxtimeadjustment
  -maxtipage=<n>
  -maxtxfee=<amt>
  -maxuploadtarget=<n>
  -mempoolexpiry=<n>
  -mempoolreplacement
  -minrelaytxfee=<amt>
  -mintxfee=<amt>
  -mocktime=<n>
  -nodebug
  -onion=<ip:port>
  -onlynet=<net>
  -par=<n>
  -paytxfee=<amt>
  -peerbloomfilters
  -permitbaremultisig
  -pid=<file>
  -port=<port>
  -printpriority
  -printtoconsole
  -privdb
  -proxy=<ip:port>
  -proxyrandomize
  -prune=<n>
  -regtest
  -reindex
  -reindex-chainstate
  -relaypriority
  -rescan
  -rest
  -rpcallowip=<ip>
  -rpcauth=<userpw>
  -rpcbind=<addr>
  -rpccookiefile=<loc>
  -rpcnamecoinapi
  -rpcpassword=<pw>
  -rpcport=<port>
  -rpcserialversion
  -rpcservertimeout=<n>
  -rpcthreads=<n>
  -rpcuser=<user>
  -rpcworkqueue=<n>
  -salvagewallet
  -seednode=<ip>
  -sendfreetransactions
  -server
  -shrinkdebugfile
  -spendzeroconfchange
  -stopafterblockimport
  -sysperms
  -testnet
  -testsafemode
  -timeout=<n>
  -torcontrol=<ip>:<port>
  -torpassword=<pass>
  -txconfirmtarget=<n>
  -txindex
  -uacomment=<cmt>
  -upgradewallet
  -usehd
  -version
  -wallet=<file>
  -walletbroadcast
  -walletnotify=<cmd>
  -walletrbf
  -walletrejectlongchains
  -whitebind=<addr>
  -whitelist=<IP address or network>
  -whitelistforcerelay
  -whitelistrelay
  -zapwallettxes=<mode>
  -zmqpubhashblock=<address>
  -zmqpubhashtx=<address>
  -zmqpubrawblock=<address>
  -zmqpubrawtx=<address>
$ ./dogecoind-1.21 -help -help-debug |grep '^  -[-a-z0-9]\+\(=.*\)\?$' |sort
  -acceptnonstdtxn
  -addnode=<ip>
  -addresstype
  -addrmantest
  -alertnotify=<cmd>
  -asmap=<file>
  -assumevalid=<hex>
  -avoidpartialspends
  -bantime=<n>
  -bind=<addr>[:<port>][=onion]
  -blockfilterindex=<type>
  -blockmaxweight=<n>
  -blockmintxfee=<amt>
  -blocknotify=<cmd>
  -blockreconstructionextratxn=<n>
  -blocksdir=<dir>
  -blocksonly
  -blockversion=<n>
  -bytespersigop
  -chain=<chain>
  -changetype
  -checkblockindex
  -checkblocks=<n>
  -checklevel=<n>
  -checkmempool=<n>
  -checkpoints
  -conf=<file>
  -connect=<ip>
  -daemon
  -datacarrier
  -datacarriersize
  -datadir=<dir>
  -dbbatchsize
  -dbcache=<n>
  -dblogsize=<n>
  -debug=<category>
  -debugexclude=<category>
  -debuglogfile=<file>
  -deprecatedrpc=<method>
  -disablewallet
  -discardfee=<amt>
  -discover
  -dns
  -dnsseed
  -dropmessagestest=<n>
  -dustrelayfee=<amt>
  -externalip=<ip>
  -fallbackfee=<amt>
  -feefilter
  -flushwallet
  -forcednsseed
  -help-debug
  -includeconf=<file>
  -incrementalrelayfee=<amt>
  -keypool=<n>
  -limitancestorcount=<n>
  -limitancestorsize=<n>
  -limitdescendantcount=<n>
  -limitdescendantsize=<n>
  -listen
  -listenonion
  -loadblock=<file>
  -logips
  -logthreadnames
  -logtimemicros
  -logtimestamps
  -maxapsfee=<n>
  -maxconnections=<n>
  -maxmempool=<n>
  -maxorphantx=<n>
  -maxreceivebuffer=<n>
  -maxsendbuffer=<n>
  -maxsigcachesize=<n>
  -maxtimeadjustment
  -maxtipage=<n>
  -maxtxfee=<amt>
  -maxuploadtarget=<n>
  -mempoolexpiry=<n>
  -minimumchainwork=<hex>
  -minrelaytxfee=<amt>
  -mintxfee=<amt>
  -mocktime=<n>
  -networkactive
  -onion=<ip:port>
  -onlynet=<net>
  -par=<n>
  -paytxfee=<amt>
  -peerblockfilters
  -peerbloomfilters
  -peertimeout=<n>
  -permitbaremultisig
  -persistmempool
  -pid=<file>
  -port=<port>
  -printpriority
  -printtoconsole
  -privdb
  -proxy=<ip:port>
  -proxyrandomize
  -prune=<n>
  -regtest
  -reindex
  -reindex-chainstate
  -rescan
  -rest
  -rpcallowip=<ip>
  -rpcauth=<userpw>
  -rpcbind=<addr>[:port]
  -rpccookiefile=<loc>
  -rpcpassword=<pw>
  -rpcport=<port>
  -rpcserialversion
  -rpcservertimeout=<n>
  -rpcthreads=<n>
  -rpcuser=<user>
  -rpcwhitelist=<whitelist>
  -rpcwhitelistdefault
  -rpcworkqueue=<n>
  -seednode=<ip>
  -segwitheight=<n>
  -server
  -settings=<file>
  -shrinkdebugfile
  -signet
  -signetchallenge
  -signetseednode
  -spendzeroconfchange
  -startupnotify=<cmd>
  -stopafterblockimport
  -stopatheight
  -sysperms
  -testnet
  -timeout=<n>
  -torcontrol=<ip>:<port>
  -torpassword=<pass>
  -txconfirmtarget=<n>
  -txindex
  -uacomment=<cmt>
  -vbparams=deployment:start:end[:min_activation_height]
  -version
  -wallet=<path>
  -walletbroadcast
  -walletdir=<dir>
  -walletnotify=<cmd>
  -walletrbf
  -walletrejectlongchains
  -whitebind=<[permissions@]addr>
  -whitelist=<[permissions@]IP address or network>
  -whitelistforcerelay
  -whitelistrelay
  -zmqpubhashblock=<address>
  -zmqpubhashblockhwm=<n>
  -zmqpubhashtx=<address>
  -zmqpubhashtxhwm=<n>
  -zmqpubrawblock=<address>
  -zmqpubrawblockhwm=<n>
  -zmqpubrawtx=<address>
  -zmqpubrawtxhwm=<n>
  -zmqpubsequence=<address>
  -zmqpubsequencehwm=<n>

These give 1 less entry than your regex, removing -?

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 11, 2021

Is it possible to set ? as environment variables ? I'm not sure if it's possible at all, shell syntax may not allow it. It would be an unusable option as environment variable.

Maybe we could keep the -? in the list to avoid a complex regex, it shouldn't have any impact.

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 11, 2021

Or simply : ^ -[a-z]+, just getting a word.

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 11, 2021

First error caught with CI tests :) I used a single space in the regex !

I replaced the grep command by grep -E '^ -[a-z]+', let me know what do you think about that.

And for the hook, you're ok for the list or you would prefer raw help menus ?

Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

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

I have left some nits inside that will improve testing with #25, otherwise I think this is better than what we have because it enables more commands.

More good news: I was worried about performance of calling this on each run, but it's only taking a couple nanoseconds more because of the process spawn, and is actually less CPU intensive than reading files and doing regexes on the man files using python, measured end-to-end. We may want to consider building this list within the build stage rather than the execution stage, because it's static and it removes some risk from running dogecoind where there may be a configuration provided in .dogecoin/dogecoin.conf, but this doesn't have to be a priority right now.

Raw data from perf:

$ sudo perf stat -x , --repeat 100 docker run -e DISABLE_WALLET=1 branch/main -help
...
120.43,msec,task-clock,1.24%,120433973,100.00,0.210,CPUs utilized
802,,context-switches,2.13%,120433973,100.00,0.007,M/sec
6,,cpu-migrations,4.80%,120433973,100.00,0.052,K/sec
5196,,page-faults,0.11%,120433973,100.00,0.043,M/sec
166625983,,cycles,0.62%,139108619,100.00,1.384,GHz
24064012,,stalled-cycles-frontend,1.87%,139108619,100.00,14.44,frontend cycles idle
31235144,,stalled-cycles-backend,3.97%,139108619,100.00,18.75,backend cycles idle
116810845,,instructions,0.17%,139108619,100.00,0.70,insn per cycle
,,,,,0.27,stalled cycles per insn
23482944,,branches,0.18%,139108619,100.00,194.986,M/sec
670528,,branch-misses,0.65%,139108619,100.00,2.86,of all branches
$ sudo perf stat -x , --repeat 100 docker run -e DISABLE_WALLET=1 pr/34 -help
...
120.46,msec,task-clock,1.16%,120460607,100.00,0.209,CPUs utilized
798,,context-switches,2.04%,120460607,100.00,0.007,M/sec
6,,cpu-migrations,4.83%,120460607,100.00,0.050,K/sec
5197,,page-faults,0.09%,120460607,100.00,0.043,M/sec
167501134,,cycles,0.66%,138843545,100.00,1.391,GHz
23808703,,stalled-cycles-frontend,1.88%,138843545,100.00,14.21,frontend cycles idle
32960599,,stalled-cycles-backend,3.57%,138843545,100.00,19.68,backend cycles idle
116834293,,instructions,0.17%,138843545,100.00,0.70,insn per cycle
,,,,,0.28,stalled cycles per insn
23499176,,branches,0.19%,138843545,100.00,195.078,M/sec
670037,,branch-misses,0.63%,138843545,100.00,2.85,of all branches

Note that not doing any operations on env at all saves 1.3ms on average:

118.96,msec,task-clock,1.06%,118959606,100.00,0.215,CPUs utilized
815,,context-switches,1.63%,118959606,100.00,0.007,M/sec
6,,cpu-migrations,4.29%,118959606,100.00,0.052,K/sec
5203,,page-faults,0.10%,118959606,100.00,0.044,M/sec
160121434,,cycles,0.63%,137637452,100.00,1.346,GHz
22443403,,stalled-cycles-frontend,1.85%,137637452,100.00,14.02,frontend cycles idle
26591736,,stalled-cycles-backend,3.80%,137637452,100.00,16.61,backend cycles idle
117010187,,instructions,0.16%,137637452,100.00,0.73,insn per cycle
,,,,,0.23,stalled cycles per insn
23530335,,branches,0.18%,137637452,100.00,197.801,M/sec
679959,,branch-misses,0.61%,137637452,100.00,2.89,of all branches

Let me know what you want to do with the 2 nits... I'd be happy to turn this into ACK if you want to save it for later

1.14.5/bullseye/entrypoint.py Outdated Show resolved Hide resolved
1.14.5/bullseye/entrypoint.py Outdated Show resolved Hide resolved
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 11, 2021

Function extracted, ready for review

@patricklodder
Copy link
Member

Since the additional commits only change code introduced with the first commit, would you mind giving those a squash?

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 11, 2021

Done

Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

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

re is currently unused and throws a linting error. Everything else 👍

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 11, 2021

10/10 with pylint

Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

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

code review & tested ACK.

Copy link
Member

@xanimo xanimo left a comment

Choose a reason for hiding this comment

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

ACK

@xanimo xanimo merged commit 39c6a20 into dogecoin:main Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regtest doesn't work as env
3 participants