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.

Sign up for GitHub

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

Jump to bottom

feat(shim): Rework shimming logic #4543

Merged
merged 16 commits into from
Dec 2, 2021
Merged

feat(shim): Rework shimming logic #4543

merged 16 commits into from
Dec 2, 2021

Conversation

rashil2000
Copy link
Member

@rashil2000 rashil2000 commented Nov 28, 2021 •
edited
Loading

The .ps1 shims are now only created when the bin entry itself is a .ps1 script.

Closes #3749
Closes #2018
Closes #4486
Closes #3850
Closes #3795
Closes #4003
Closes #3890
Closes #4216
Closes #4473
Closes #3699
Closes #4413
Closes #4545
Closes #3851

Closes #4333
Closes #3877
Closes #4376

Related #3915
Related #2230

rasa
rasa reviewed Nov 28, 2021
View reviewed changes
lib/core.ps1 Show resolved Hide resolved
@rashil2000
Copy link
Member Author

rashil2000 commented Nov 29, 2021 •
edited
Loading

The scoop which functionality in this PR is currently broken (it always points to the shim directory for batch scripts), as it always relied on a .ps1 file being present.

@niheaven is there a way to extract the app path from batch scripts (similar to .exe/.shim and .ps1 files)?

niheaven
niheaven reviewed Nov 29, 2021
View reviewed changes
libexec/scoop-which.ps1 Outdated Show resolved Hide resolved
@rashil2000
Copy link
Member Author

@niheaven this is ready for review.

niheaven
niheaven reviewed Nov 30, 2021
View reviewed changes
Copy link
Member

@niheaven niheaven left a comment

Choose a reason for hiding this comment

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

The ps1 shim also has PATH comment, right?

libexec/scoop-which.ps1 Outdated Show resolved Hide resolved
rasa
rasa reviewed Dec 1, 2021
View reviewed changes
lib/core.ps1 Show resolved Hide resolved
niheaven
niheaven requested changes Dec 2, 2021
View reviewed changes
libexec/scoop-which.ps1 Outdated Show resolved Hide resolved
libexec/scoop-which.ps1 Show resolved Hide resolved
libexec/scoop-which.ps1 Outdated Show resolved Hide resolved
@rashil2000
Copy link
Member Author

Select-Object is used because it can fail gracefully.

Using [] will throw an error if the index (or the array itself) doesn't exist (which will be the case for older existing ps1 shims), and then we'll have to use a try-catch block or something other to first detect the error and then write a fallback.

In case of Select-Object, we can directly use a fallback (as is done here).

@niheaven
Copy link
Member

niheaven commented Dec 2, 2021

I've changed two expression that use regex to clarify what we really want.

@rashil2000
Copy link
Member Author

Done

@niheaven
Copy link
Member

niheaven commented Dec 2, 2021

Let's begin testing it!

@niheaven niheaven merged commit ef4349b into develop Dec 2, 2021
@niheaven niheaven deleted the ps1-shim branch December 2, 2021 13:23
This was referenced Dec 2, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@chawyehsu
Copy link
Member

you shouldn't close those issues when the fix hasn't been shipped to master...

@rashil2000
Copy link
Member Author

I closed them with the idea that people can test the develop branch if they want to

@chawyehsu
Copy link
Member

chawyehsu commented Dec 3, 2021 •
edited
Loading

You can notify them by just leaving a comment instead of directly closing the issue which is not fixed “officially”...

@niheaven niheaven mentioned this pull request Dec 24, 2021
Merged
niheaven added a commit that referenced this pull request Dec 25, 2021
@niheaven @rashil2000
@Ash258 @rasa @filmor @MrNuggelz @pratikpc @nickbudi @jfastnacht @tukanos
Merge branch 'develop' into 'master' (#4594)
1dbab1f 
Related PRs:
- #4531 @filmor 
  - ac71fcc @niheaven 
- #4535 @rashil2000 
- #4522 @pratikpc 
- #4550 @niheaven 
- #4528 @niheaven 
- #4532 @MrNuggelz
- #4155 @MrNuggelz Co-authored-by: @rashil2000 
  - #4581 @niheaven 
  - fb496c4 @rashil2000 
- #4543 @rashil2000 Co-authored-by: @niheaven 
  - #4555 @rashil2000 
  - 3c90d1a @rashil2000 
  - 2ec00d5 @rashil2000 
- #4567 @rashil2000 
  - cbe29ed @rashil2000 
- #4570 @niheaven 
  - #4582 @niheaven 
- #4571 @niheaven 
- #3244 @nickbudi 
- #3821 @jfastnacht Co-authored-by: @rasa 
- #4578 @tukanos
- #4579 @rashil2000 

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
Co-authored-by: Jakub Čábera <cabera.jakub@gmail.com>
Co-authored-by: Ross Smith II <ross@smithii.com>
Co-authored-by: Benedikt Reinartz <filmor@gmail.com>
Co-authored-by: Joris <MrNuggelz@users.noreply.github.com>
Co-authored-by: Pratik Chowdhury <pratikc@live.co.uk>
Co-authored-by: Rashil Gandhi <rashil2000@gmail.com>
Co-authored-by: nickbudi <nickbudi@users.noreply.github.com>
Co-authored-by: Julian <github@fastnacht.consulting>
Co-authored-by: tukanos <patrik.svestka@gmail.com>
@adamency
Copy link

adamency commented Jan 17, 2022 •
edited
Loading

How is this related to #2230 ?

@rashil2000
Copy link
Member Author

It concerns shims

@niheaven
Copy link
Member

Does this fix #2230 and #3915? What should we do now?

@rashil2000
Copy link
Member Author

Does this fix #2230 and #3915? What should we do now?

I think that problem can be solved by using relative paths in batch and bash shims too (ps1 shim already uses relative paths). But it will require careful testing, and rework of scoop-which also.

@niheaven
Copy link
Member

I think that problem can be solved by using relative paths in batch and bash shims too (ps1 shim already uses relative paths). But it will require careful testing, and rework of scoop-which also.

Please take your times to do the jobs.

se35710 pushed a commit to se35710/scoop that referenced this pull request Mar 8, 2022
@rashil2000 @niheaven @se35710
feat(shim): Rework shimming logic (ScoopInstaller#4543)
c7ba268 
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
se35710 pushed a commit to se35710/scoop that referenced this pull request Mar 8, 2022
@niheaven @rashil2000
@Ash258 @rasa @filmor @MrNuggelz @pratikpc @nickbudi @jfastnacht @tukanos @se35710
Merge branch 'develop' into 'master' (ScoopInstaller#4594)
e334b81 
Related PRs:
- ScoopInstaller#4531 @filmor
  - ac71fcc @niheaven
- ScoopInstaller#4535 @rashil2000
- ScoopInstaller#4522 @pratikpc
- ScoopInstaller#4550 @niheaven
- ScoopInstaller#4528 @niheaven
- ScoopInstaller#4532 @MrNuggelz
- ScoopInstaller#4155 @MrNuggelz Co-authored-by: @rashil2000
  - ScoopInstaller#4581 @niheaven
  - fb496c4 @rashil2000
- ScoopInstaller#4543 @rashil2000 Co-authored-by: @niheaven
  - ScoopInstaller#4555 @rashil2000
  - 3c90d1a @rashil2000
  - 2ec00d5 @rashil2000
- ScoopInstaller#4567 @rashil2000
  - cbe29ed @rashil2000
- ScoopInstaller#4570 @niheaven
  - ScoopInstaller#4582 @niheaven
- ScoopInstaller#4571 @niheaven
- ScoopInstaller#3244 @nickbudi
- ScoopInstaller#3821 @jfastnacht Co-authored-by: @rasa
- ScoopInstaller#4578 @tukanos
- ScoopInstaller#4579 @rashil2000

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
Co-authored-by: Jakub Čábera <cabera.jakub@gmail.com>
Co-authored-by: Ross Smith II <ross@smithii.com>
Co-authored-by: Benedikt Reinartz <filmor@gmail.com>
Co-authored-by: Joris <MrNuggelz@users.noreply.github.com>
Co-authored-by: Pratik Chowdhury <pratikc@live.co.uk>
Co-authored-by: Rashil Gandhi <rashil2000@gmail.com>
Co-authored-by: nickbudi <nickbudi@users.noreply.github.com>
Co-authored-by: Julian <github@fastnacht.consulting>
Co-authored-by: tukanos <patrik.svestka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rasa rasa rasa left review comments

@niheaven niheaven niheaven approved these changes

@issaclin32 issaclin32 issaclin32 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rashil2000 @niheaven @chawyehsu @adamency @rasa @issaclin32