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

feat(core): support loading basedirs from config #3121

Merged
merged 3 commits into from
May 10, 2019

Conversation

chawyehsu
Copy link
Member

@chawyehsu chawyehsu commented Feb 13, 2019

Reopen #3116 . This is a breaking change.

@chawyehsu
Copy link
Member Author

chawyehsu commented Feb 14, 2019

please also add please-review label. Once this pull request is accepted, ScoopInstaller/Install#3 will also be accepted and merged. And the new installer will be ready to use for develop channel users.

@r15ch13 r15ch13 added the review-needed Asks for review of these changes label Feb 14, 2019
@chawyehsu
Copy link
Member Author

/cc @r15ch13 @Ash258 @rasa

@Ash258
Copy link
Contributor

Ash258 commented Mar 4, 2019

I am not against this change in overall, but i like that idea, when you can adress scoop location in other scripts / programs using $env:SCOOP(_HOME). I usually use it a lot and it's more convenient to guide user (for example) using cd $env:SCOOP rather than cd (scoop config rootPath).

Should be global path set in .scoop file too?? Each users will have to set it on user creation / .. to match other users. Global path as machine environment variable is more suitable I think.

@chawyehsu
Copy link
Member Author

@Ash258 It's ok to keep $env:SCOOP for users to adress scoop location. I prefer to get it from scoop config rootPath and set it to $env:SCOOP in the scoop runtime for users, instead of saving it to and getting it from environment variable. Will consider to tweak the implementation.

@r15ch13
Copy link
Member

r15ch13 commented Mar 12, 2019

Using $env: and config values is a good solution, but which one has priority over the other?
Keep in mind that this might break scoop-extras/checkver.ps1 and other bin scripts.

@chawyehsu
Copy link
Member Author

@r15ch13

  1. I think config values should have higher priority, and $env:SCOOP is an 'alias' to scoop config rootPath.(Just, don't write environment variable if you can, please..) An user can get scoop path from $env:SCOOP but can only change the path from scoop config (but hey, is it really someone will change the scoop path after installation?)

  2. It's ok.

    f(!$env:SCOOP_HOME) { $env:SCOOP_HOME = resolve-path (split-path (split-path (scoop which scoop))) }

@Ash258

Global path as machine environment variable is more suitable I think.

Agree.

@chawyehsu chawyehsu changed the title migrating core environment variable to config file feat: support loading env from scoop config file Mar 15, 2019
@chawyehsu
Copy link
Member Author

Now the changes is non-invasive, just added support of loading env from ~/.scoop config file, to reflect the future standalone installer.

@chawyehsu
Copy link
Member Author

Anyone else wants to give a review?

lib/core.ps1 Outdated Show resolved Hide resolved
@niheaven
Copy link
Member

niheaven commented Mar 18, 2019

Reviewed and tested without reinstallation of Scoop. Online installation failed.

Rebased with #3242, but get_config is defined after $scoopdir and is unreachable.

After changing sequence of $scoopdir and get_config, online installation work well. (test branch and online install script, please see additional commit there).

@h404bi

@chawyehsu
Copy link
Member Author

@niheaven Thanks for respone and test, will rebase the codes after getting #3242 adopted.

@chawyehsu chawyehsu changed the base branch from master to develop May 10, 2019 02:39
@chawyehsu chawyehsu changed the title feat: support loading env from scoop config file feat(core): support loading basedirs from config May 10, 2019
@r15ch13 r15ch13 merged commit 89558f4 into ScoopInstaller:develop May 10, 2019
@chawyehsu chawyehsu deleted the patch-9 branch May 13, 2019 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-needed Asks for review of these changes work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants