Add envvar system #16829

pull emilengler wants to merge 3 commits into bitcoin:master from emilengler:2019-09-envvar changing 9 files +79 −0
  1. emilengler commented at 2:09 pm on September 8, 2019: contributor

    Description

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

    Testing

    Set some envvars On Unix:

    0BITCOIND_DATADIR=/path/to/your/datadir
1BITCOIND_LANG=en
2export BITCOIND_DATADIR
3export BITCOIND_LANG
4
5./bitcoin-qt

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

    Notes

    This also adds a function to ArgsManager to get all added args in a vector.

  2. fanquake renamed this:
    trivial: Add envvar system
    Add envvar system
    on Sep 8, 2019
  3. fanquake added the label Feature on Sep 8, 2019
  4. fanquake added the label Needs Conceptual Review on Sep 8, 2019
  5. fanquake requested review from ryanofsky on Sep 8, 2019
  6. Add envvar system 0849eabfd6
  7. Fix whitespace 
    Fix whitespaces
    a294e1b3a5
  8. DrahtBot commented at 9:32 pm on September 9, 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:

    • #16545 (Implement missing error checking for ArgsManager flags by ryanofsky)

    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.

  9. laanwj commented at 7:16 am on September 10, 2019: member
    Concept ~0 Although I agree this is useful for some settings—especially the datadir, for language there’s already LANG—which should be picked up by Qt) I very much dread adding another source of configuration information. That logic is already complicated, especially for the GUI which deals with the QSettings. I think that was the rationale for rejecting this before, but maybe things have changed.
  10. elichai commented at 11:11 am on September 10, 2019: contributor
    Could you show a concrete important use case that this solve that can’t be solved with flags/conf file? Because the more conf sources we have the complicated it is to stay consistent on what overrides what and making sure everything makes sense.
  11. MarcoFalke commented at 11:18 am on September 10, 2019: member
    If this is done, it should probably happen after #15934, which clarifies the settings merging a bit.
  12. emilengler commented at 4:17 pm on September 10, 2019: contributor
    @laanwj How QSettings can mess up with it? The envvars are ‘higher’ than QSetttings.
  13. laanwj commented at 4:47 pm on September 10, 2019: member

    Exactly because of the precedence orders (and exceptions to it, eg datadir is treated specially in the ui). Don’t know if QSettings specifically can mess with it, but fact is this creates even more configuration possibilities and wouldn’t be surprised if some interaction broke.

    On Tue, Sep 10, 2019, 18:17 Emil Engler notifications@github.com wrote:

    @laanwj https://github.com/laanwj How QSettings can mess up with it? The envvars are ‘higher’ than QSetttings.

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/16829?email_source=notifications&email_token=AAA65NUJJ6XYVK5SDLAG763QI7CCRA5CNFSM4IUTSBE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6LVQ7Q#issuecomment-530012286, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA65NXRAZWIZNG3JBAX6PLQI7CCRANCNFSM4IUTSBEQ .

  14. ryanofsky commented at 8:37 pm on September 10, 2019: member
    What’s a practical use-case for this? I’d expect this to be more useful for a tool like git or bitcoin-cli where you’re calling a tool repeatedly or in a script than for a daemon or gui application.
  15. fanquake commented at 6:24 am on September 11, 2019: member

    I very much dread adding another source of configuration information.

    What’s a practical use-case for this?

    Agree with this two comments. I’m a Concept NACK unless we get some good use cases that outweigh adding another way to configure Bitcoin Core.

  16. elichai commented at 9:46 am on September 11, 2019: contributor

    @ryanofsky You can do: A:

    0$ export BITCOIN_FLAGS='--datadir=~/.bitcoin2 --rpcuser=something --rpcwallet=secret'
1
2$ bitcoin-cli $BITCOIN_FLAGS sendtoaddress XXX 1 
3$ bitcoin-cli $BITCOIN_FLAGS listunspent 0

    B:

    0$ alias bitcoin-cli='bitcoin-cli --datadir=~/.bitcoin2 --rpcuser=something --rpcwallet=secret'
1
2$ bitcoin-cli sendtoaddress XXX 1 
3$ bitcoin-cli listunspent 0
  17. ryanofsky commented at 10:59 am on September 11, 2019: member

    @elichai, thanks for teaching me how the unix shell works :wink:, but I still think environment variables are useful when you want to configure command line and scriptable utilities like bitcoin-cli or git.

    For one thing there are shells other than the unix shell. For example, I wouldn’t be surprised if there were as many users invoking bitcoin-cli from the windows command line as from the unix shell, and wanting to be able to simply set the bitcoin datadir through an environment variable in the control panel, so it is picked up and used reliably across the system without needing to write long command lines or wrapper scripts.

    For another thing, unlike aliases, environment variables work across processes rather than just inside a single shell. So if you have a script calling bitcoin-cli, environment variables set before the script can be used to configure bitcoin-cli without having to edit the the script. And this works not only for scripts invoked directly, but scripts invoked indirectly, such as in a -walletnotify handler.

    Another example would be very awkward interaction between the command line and config file -datadir settings in systemd, where a datadir set in /etc/bitcoin/bitcoin.conf is silently ignored due to command line precedence (https://github.com/bitcoin/bitcoin/pull/12255#pullrequestreview-181439636). Introducing an environment variable and new config precedence as suggested there would remove this footgun (though in this case, we need the environment variable to have less precedence than the config value, not greater, which is why I suggested BITCOIN_HOME, and not BITCOIN_DATADIR there).

    TL;DR. While I tend to agree with Concept NACKs on this feature, I do actually want to know what motivated @emilengler to work on this PR, and if there are real use-cases that this change or another more limited change might be able to solve. (More limited versions of this PR might be only using environment variables for bitcoin-cli, and not bitcoind and bitcoin-qt. Or only interpretting a single environment variable like BITCOIN_DATADIR or BITCOIN_CONF or BITCOIN_OPTS rather than dozens of different obscure variables.)

  18. elichai commented at 11:08 am on September 11, 2019: contributor

    @ryanofsky obviously I know that you know unix shell quite good :) Just tried to drill down the exact use case for where it diverge from regular shell stuff.

    I agree with you, curious to hear the use case (which is why I asked exactly that haha)

  19. Add support for envvars in bitcoin-cli c8470c3ed0
  20. emilengler commented at 2:23 pm on September 11, 2019: contributor
    @ryanofsky The usecase was like you sad for bitcoin-cli. I actually forget this when working on this. Added it now
  21. laanwj commented at 6:08 am on September 12, 2019: member

    Or only interpretting a single environment variable like BITCOIN_DATADIR or BITCOIN_CONF

    I like this solution. I do see the value of overriding the default datadir / configuration path through a environment option, especially (but not only) for bitcoin-cli. E.g. On some nodes I have the data directory in another path because it doesn’t fit on the main drive, and am using aliases for that right now, but the drawback is that it doesn’t propagate to scripts.

    So ACK on adding BITCOIN_DATADIR and BITCOIN_CONF. NACK on making arbitrary options overridable.

  22. emilengler commented at 4:30 pm on September 12, 2019: contributor
    @laanwj I also thougt about this but I thought this would get NACKed because it is too specific. But it looks like this more popular. Will close this in the favor of @ryanofsky’s suggestion and work on this
  23. emilengler closed this on Sep 12, 2019
  24. emilengler deleted the branch on Sep 12, 2019
  25. emilengler commented at 11:42 am on September 14, 2019: contributor
    See #16868
  26. DrahtBot locked this on Dec 16, 2021


emilengler DrahtBot laanwj elichai MarcoFalke ryanofsky fanquake


ryanofsky

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-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me