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

Add specific envvar system #16868

Closed

Conversation

@emilengler
Copy link
Contributor

commented Sep 13, 2019

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:

export BITCOIN_DATADIR=/path/to/your/datadir
export BITCOIN_CONF=/path/to/your/conf
./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
@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

The travis exception is there because all characters there are valid English Latin ASCII chars so there won't be any problems.

@emilengler emilengler referenced this pull request Sep 14, 2019
@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Ping @ryanofsky

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

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.

@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@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.
@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@ryanofsky Done

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

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?)

@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@MarcoFalke How?
Config files or cmd args have a higher priority than envvars.
See travis, no errors

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

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.

@emilengler emilengler force-pushed the emilengler:2019-09-specific-envvar branch from 7485703 to 0a76662 Sep 21, 2019
@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Added help

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Concept ACK on BITCOIN_DATADIR an BITCOIN_CONF. I do not believe any of the others are necessary.

@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Ok, then I will remove BITCOIN_PID

@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Done 6ccb229

@fanquake

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

@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.

@emilengler emilengler force-pushed the emilengler:2019-09-specific-envvar branch from b968a3a to b93d93e Sep 26, 2019
@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

I removed the help from the -help command to the docs because this was only causing a cricular dependency

@emilengler emilengler force-pushed the emilengler:2019-09-specific-envvar branch from 11c9828 to ad32e87 Sep 26, 2019
@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@fanquake Done

Name | Option equivalent
-- | --
BITCOIN_DATADIR | -datadir
BITCOIN_CONF | -conf

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 26, 2019

Member

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.

This comment has been minimized.

Copy link
@emilengler

emilengler Sep 26, 2019

Author Contributor

It doesn't really complicates the init process. It just checks if a specific argument is set. If not it will use the envvar

This comment has been minimized.

Copy link
@emilengler

emilengler Sep 26, 2019

Author Contributor

@MarcoFalke Updated, please check

This commit adds a way to set the -conf and -datadir flag using the environment variables BITCOIN_CONF and BITCOIN_DATADIR
@emilengler emilengler force-pushed the emilengler:2019-09-specific-envvar branch from ad32e87 to 6cea457 Sep 26, 2019
@emilengler

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

Closed because of lack of interest

@emilengler emilengler closed this Oct 14, 2019
@emilengler emilengler deleted the emilengler:2019-09-specific-envvar branch Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.