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(path): Isolate Scoop apps' PATH #5840

Merged
merged 20 commits into from
Apr 18, 2024
Merged

feat(path): Isolate Scoop apps' PATH #5840

merged 20 commits into from
Apr 18, 2024

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Mar 22, 2024

Description

Separate from #5836, will rebase on develop when #5836 is merged.

Use the use_isolated_path config option to separate Scoop apps PATH to SCOOP_PATH (or any other name), then add %SCOOP_PATH% to $env:Path when installing apps with env_add_path for the first time.

Motivation and Context

This PR does the following:

  • Use -TargetEnvVar in Add-Path() and Remove-Path() to add/remove path in different EnvVars
  • Check use_isolated_path in env_add_path() and env_rm_path() to add/remove apps path in %SCOOP_PATH% or %PATH%
  • Add %SCOOP_PATH% when first add path to it

Path switching is done with Complete-ConfigChange().

How Has This Been Tested?

Passed scoop test.
image

%SCOOP_PATH% is added correctly:
image
image

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

Base automatically changed from refactor-system to develop March 27, 2024 09:32
lib/system.ps1 Outdated Show resolved Hide resolved
@chawyehsu
Copy link
Member

Is it a replacement of #5813?

@niheaven
Copy link
Member Author

Is it a replacement of #5813?

Yes, after reviewing the above PR, I found a simpler and clearer way to implement the feature. This was especially true after refactoring the EnvVar-related functions.

Initially, I attempted to modify pull request #5813, but ultimately decided it would be easier to create a new one instead.

lib/install.ps1 Outdated Show resolved Hide resolved
@niheaven niheaven force-pushed the feat-isolate-path branch 2 times, most recently from afd93ff to b0f3d0d Compare April 3, 2024 03:31
lib/core.ps1 Outdated Show resolved Hide resolved
lib/install.ps1 Outdated Show resolved Hide resolved
lib/install.ps1 Outdated Show resolved Hide resolved
@chawyehsu
Copy link
Member

I think we should use isolated path just by default (instead of making it a config option)

We need a transition period.

@rashil2000
Copy link
Member

What for, if I may ask?
The scoop update command can handle the environment change without user intervention, right? It doesn't seem to be a breaking change.

@niheaven
Copy link
Member Author

By setting SCOOP_PATH as default, I will make a commit and do migration as Complete-ConfigChange().

@chawyehsu
Copy link
Member

As far as I'm concerned it is a breaking change from the implementation level. But since it has changed to force this behavoir I don't talk about it. My review on the PR (latest commits not included) has done previously.

@niheaven
Copy link
Member Author

Got the idea. So it's better to offer config now and notify users that we'll change the default behavior and change it a few months later.

What's your opinion? @chawyehsu @rashil2000

@rashil2000
Copy link
Member

I don't have an issue with providing a config option per se - I just don't see purpose in involving user interaction in an internal implementation detail.

@niheaven
Copy link
Member Author

So choose an implementation here and then we can start bumping the version.

If config, I'll revert the last commit and wait for several months to submit a PR again, if default, then directly merge all commits.

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

So, I'm facing a weird error:
image

Did you face/rectify this?

@rashil2000
Copy link
Member

In addition to $scoopdir\apps\* paths, the SCOOP_PATH should also contain $scoopdir\shims, right? I think we missed it here...

@niheaven
Copy link
Member Author

No, putting $scoopdir\shims directly in PATH should be preferred. This will not break scoop's feature in case of SCOOP_PATH-related error.

@CEbbinghaus
Copy link

For convenience, a fixed name is better for users.

I agree, it's sufficient and self-explanatory.

I am not arguing that it is not self explanatory. I am saying that limiting it to 1 single environment variable breaks peoples environments who use that environment variable for other purposes (e.g setting the path to the scoop executable). It is fairly easy to allow configuration of the environment variable so its not a huge cost while the benefit to end users might be not having to fix their setup.

@chawyehsu
Copy link
Member

use that environment variable for other purposes (e.g setting the path to the scoop executable)

SCOOP_PATHS/SCOOP_APPPATHS would be a better name if you concern about this, I don't think SCOOP_PATHS/SCOOP_APPPATHS has been used elsewhere. It is feasible to allow variable env var with something like use_isolated_path: "MY_PERFECT_ENV_NAME" but I'm not a fan of this kind of flexibility.

What's your opinion?

So choose an implementation here and then we can start bumping the version.

If config, I'll revert the last commit and wait for several months to submit a PR again, if default, then directly merge all commits.

Abstention

@niheaven
Copy link
Member Author

Tired of this conversation. I'll save the current branch and reset it to the config implementation to avoid a significant user experience change. I'll also remind the user that we'll do the migration in six months.

@niheaven
Copy link
Member Author

All done, please review again.

@niheaven niheaven merged commit 5819b5a into develop Apr 18, 2024
2 checks passed
@niheaven niheaven deleted the feat-isolate-path branch April 18, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants