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

Parse a configuration file #274

Closed
wants to merge 1 commit into from
Closed

Parse a configuration file #274

wants to merge 1 commit into from

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Dec 4, 2017

I really like the installer, but I would prefer if it would use a different directory by default.
This PR allows the installer script to read a configuration file and thus have permanent configuration settings. I know that I could write a small wrapper around the installer or simply patch it, but I believe that I am not the only one having this problem.

tl;dr: With this PR, one can define ~/.config/dlang/installer.cfg and do sth. like:

ROOT=~/.dlang

To use a different default directory.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@@ -124,17 +124,19 @@ check_tools() {

# ------------------------------------------------------------------------------

mkdir -p "$ROOT"
TMP_ROOT=$(mktemp -d "$ROOT/.installer_tmp_XXXXXX")
init() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be done after the configuration and CLI arguments are read.
Note that at the moment even if --path was set, the temporary directory was ~/dlang/.installer_tmp.

@codecov-io
Copy link

codecov-io commented Dec 4, 2017

Codecov Report

Merging #274 into master will decrease coverage by 0.2%.
The diff coverage is 56.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
- Coverage   69.81%   69.61%   -0.21%     
==========================================
  Files           1        1              
  Lines         328      362      +34     
==========================================
+ Hits          229      252      +23     
- Misses         99      110      +11
Impacted Files Coverage Δ
script/install.sh 69.61% <56.41%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5341de...e2475bb. Read the comment docs.

@MartinNowak
Copy link
Member

Interesting, what other config options come to mind?
We should definitely fix the --path installation.

We could infer the install path from the install.sh location, seems ergonomically better than a config.

@wilzbach
Copy link
Member Author

Interesting, what other config options come to mind?

force-gpg maybe?
no-auto-update (If you want to to do auto-updates of the installer, this would be an option too.

If we start to offer backup mirrors for the downloads, that could be a config as well.

We could infer the install path from the install.sh location, seems ergonomically better than a config.

Well there are use cases where someone wants to use a different download folder for the binaries than the location of the script, but yes I think using the script's directory would be a very reasonable default.

@MartinNowak
Copy link
Member

A different path could easily be configured by symlinking ~/dlang to wherever you want.
I'd certainly say that ~/my/path/install.sh should default to use ~/my/path.
The rest of the config options seem vague, so a config file adds complexity and seems unwarranted atm.

@wilzbach
Copy link
Member Author

A different path could easily be configured by symlinking ~/dlang to wherever you want.
I'd certainly say that ~/my/path/install.sh should default to use ~/my/path.

We can't do that without breaking changes.
Currently the location is always:

ROOT=~/dlang

Who knows how people depend on this?

The rest of the config options seem vague, so a config file adds complexity and seems unwarranted atm.

Always updating the installer (#302) is something you requested yourself ;-)
And there we should give people an easy way to opt out. Of course, environment variables would do the job too, but they don't scale well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants