Add specific envvar system #16868

pull emilengler wants to merge 1 commits into bitcoin:master from emilengler:2019-09-specific-envvar changing 11 files +75 −0
  1. emilengler commented at 10:55 pm on September 13, 2019: contributor

    See #16829

    Description

    This PR adds a way to set some CLI args with an environment variable. The syntax for the envvars are BITCOIN_<ARGNAME>=<VALUE> E.g BITCOIN_DATADIR=/home/emil/bitcoin/ Console arguments override setted envvars. The envvars are always uppercase.

    Testing

    Set some envvars On Unix:

    0export BITCOIN_DATADIR=/path/to/your/datadir
1export BITCOIN_CONF=/path/to/your/conf
2./bitcoin-qt

    This will start Bitcoin-Qt using the datadir path with an english overlay.

    List of supported envvars

    • datadir
    • conf
    • blocksdir
    • debuclogfile
    • includeconf
    • loadblock
    • pid
    • Others can be added by changing one line of code
  2. fanquake added the label Feature on Sep 13, 2019
  3. fanquake added the label Needs Conceptual Review on Sep 13, 2019
  4. emilengler commented at 11:09 pm on September 13, 2019: contributor
    The travis exception is there because all characters there are valid English Latin ASCII chars so there won’t be any problems.
  5. in test/lint/lint-locale-dependence.sh:23 in ea9053f9f4 outdated 
    19@@ -20,7 +20,7 @@ KNOWN_VIOLATIONS=(
20     "src/util/system.cpp:.*atoi"
21 )
22 
23-REGEXP_IGNORE_EXTERNAL_DEPENDENCIES="^src/(crypto/ctaes/|leveldb/|secp256k1/|tinyformat.h|univalue/)"
24+REGEXP_IGNORE_EXTERNAL_DEPENDENCIES="^src/(crypto/ctaes/|leveldb/|secp256k1/|tinyformat.h|univalue/|envvar.cpp)"
    meshcollider commented at 3:38 am on September 15, 2019:
    This is the wrong place to put the ignore, it isn’t an external dependency it is a known violation (above)
    emilengler commented at 12:26 pm on September 15, 2019:
    Fixed see 912854784b11aadb907287a78748c3f75460c682
  6. emilengler commented at 4:08 pm on September 16, 2019: contributor
    Ping @ryanofsky
  7. ryanofsky commented at 5:37 pm on September 16, 2019: member

    Two issues I see and suggested fixes I’d make:

    1. No documentation. Environment variables interpreted by bitcoin should be part of man pages and probably -help output. Suggested fix: explicitly list environment variables that affect bitcoin settings in relevant documentation, and say what the precedence is (seems to be less than command line settings and more than static config settings, which I think is a good choice).

    2. Too many environment variables are read. I think the only environment variables that should be read are BITCOIN_DATADIR are BITCOIN_CONF, because these are sufficient to bring in full configurations, and because exposing more variables creates chances for confusion and inconsistency (why is BITCOIN_BLOCKSDIR interpreted but BITCOIN_WALLETDIR ignored?), bikeshedding, future incompatibility across bitcoin versions, and silently ignored configuration errors. Suggested fix: stop interpreting other variables besides BITCOIN_DATADIR and BITCOIN_CONF.

    Probably if this is going to be merged there should also be a python test covering this functionality. But you might could wait for more concept acks before putting work into that.

    I guess I don’t feel strongly, but overall I’d be for this change with rough edges removed.

  8. emilengler commented at 5:48 pm on September 16, 2019: contributor

    @ryanofsky

    1. Sure, I will write the documentation later.
    2. I added all variables which take a file as an argument. Sure I will remove the others because they are unnecessary variables. However I still think that interpreting pid is ok because it is often used as an argument by init systems.
  9. emilengler commented at 7:47 pm on September 16, 2019: contributor
    @ryanofsky Done
  10. MarcoFalke commented at 11:00 am on September 18, 2019: member

    what the precedence is (seems to be less than command line settings and more than static config settings, which I think is a good choice).

    This will break the functional (and possibly unit tests?)

  11. emilengler commented at 6:39 pm on September 18, 2019: contributor
    @MarcoFalke How? Config files or cmd args have a higher priority than envvars. See travis, no errors
  12. MarcoFalke commented at 7:03 pm on September 18, 2019: member

    Config files or cmd args have a higher priority than envvars.

    Would be nice to document this

    See travis, no errors

    No environment variables are set on travis, so it can’t possibly fail

  13. DrahtBot commented at 8:26 pm on September 18, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16659 (refactoring: Remove unused includes by practicalswift)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  14. emilengler commented at 8:35 pm on September 24, 2019: contributor
    Added help
  15. laanwj commented at 9:45 am on September 25, 2019: member
    Concept ACK on BITCOIN_DATADIR an BITCOIN_CONF. I do not believe any of the others are necessary.
  16. emilengler commented at 1:17 pm on September 25, 2019: contributor
    Ok, then I will remove BITCOIN_PID
  17. emilengler commented at 1:20 pm on September 25, 2019: contributor
    Done 6ccb229
  18. fanquake commented at 8:38 am on September 26, 2019: member
    @emilengler Can you squash your chanegs into a single commit (dropping all intermediate commit messages etc), add a more descriptive commit message and body as well as update the PR text to reflect the current state of this PR.
  19. fanquake added the label Waiting for author on Sep 26, 2019
  20. emilengler commented at 4:02 pm on September 26, 2019: contributor
    I removed the help from the -help command to the docs because this was only causing a cricular dependency
  21. emilengler commented at 4:09 pm on September 26, 2019: contributor
    @fanquake Done
  22. in doc/bitcoin-conf.md:74 in ad32e87506 outdated 
    64+## Environment variables
65+
66+Name | Option equivalent
67+-- | --
68+BITCOIN_DATADIR | -datadir
69+BITCOIN_CONF	| -conf
    MarcoFalke commented at 4:17 pm on September 26, 2019:
    This complicates the init process and argument parsing. I am ~0 on the overall pull, but at the very least, this should include documentation on the precedence. Tests wouldn’t hurt either.
    emilengler commented at 4:21 pm on September 26, 2019:
    It doesn’t really complicates the init process. It just checks if a specific argument is set. If not it will use the envvar
    emilengler commented at 5:52 pm on September 26, 2019:
    @MarcoFalke Updated, please check
  23. Set datadir and conf with an environment variable 
    This commit adds a way to set the -conf and -datadir flag using the environment variables BITCOIN_CONF and BITCOIN_DATADIR
    6cea4576e5
  24. emilengler commented at 3:27 pm on October 14, 2019: contributor
    Closed because of lack of interest
  25. emilengler closed this on Oct 14, 2019
  26. emilengler deleted the branch on Oct 14, 2019
  27. laanwj removed the label Waiting for author on Oct 24, 2019
  28. MarcoFalke locked this on Dec 16, 2021


emilengler meshcollider ryanofsky MarcoFalke DrahtBot laanwj fanquake

Labels
Feature Needs Conceptual Review

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me