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

perf(scoop): Load libs only once #4839

Merged
merged 7 commits into from
Apr 21, 2022
Merged

perf(scoop): Load libs only once #4839

merged 7 commits into from
Apr 21, 2022

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Mar 23, 2022

Description

Only import libs once, and necessary libs (e.g. core.ps1, buckets.ps1, commands.ps1 and help.ps1) are sourced in bin\scoop.ps1.

Also refactor bin\scoop.ps1 file and fix help system. Now scoop help <cmd>, scoop <cmd> --help, scoop <cmd> -h, scoop <cmd> /? give subcommand help, and scoop, scoop help, scoop --help, scoop -h, scoop /? give scoop help.

Closes #3418

How Has This Been Tested?

Tests are passed, and all command w/o args haven't error. More tests (subcommands') are needed.

Checklist:

  • I have read the Contributing Guide.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@niheaven
Copy link
Member Author

Any idea? @chawyehsu @issaclin32 @rashil2000 ?

@rashil2000
Copy link
Member

bin/scoop.ps1 should check for presence of git before running git commands (or have a fallback).

@niheaven
Copy link
Member Author

What is current behavior? Sorry I haven't my Windows machine nearby, and cannot test if 'git' is not present...

scoop command itself use git only in scoop version, and maybe if no 'git', get scoop version from CHANGELOG.md is a viable choice?

@rashil2000
Copy link
Member

image

get scoop version from CHANGELOG.md is a viable choice?

Sounds good

@rashil2000
Copy link
Member

  • We're still not checking for presence of git when returning scoop's version
  • Does the order of imports in the files matter? I see that you've moved many imports up and down. If the order is important, LGTM. Otherwise we should not change it, to keep git history clean.

@niheaven
Copy link
Member Author

We're still not checking for presence of git when returning scoop's version

Yes, I've almost done it, but I've been blocked in my home (damn COVID-19) and the coding process may be a little slower.

  • Does the order of imports in the files matter? I see that you've moved many imports up and down. If the order is important, LGTM. Otherwise we should not change it, to keep git history clean.

It doesn't matter, and the order here is getopt.ps1 first, others are just modified by adding some comments. In test files, Scoop-TestLib.ps1 first and unix.ps1 last.

This PR should be done in a few days, I hope 😂

@rashil2000
Copy link
Member

Hey, no worries! Personal lives always come first :)

@niheaven
Copy link
Member Author

All done, IMO.

@rashil2000
Copy link
Member

Will review later today

rashil2000
rashil2000 previously approved these changes Apr 20, 2022
bin/scoop.ps1 Outdated Show resolved Hide resolved
Co-authored-by: Rashil Gandhi <[email protected]>
@niheaven niheaven merged commit 6296822 into develop Apr 21, 2022
@niheaven niheaven deleted the feature/perf-scoop branch April 21, 2022 13:34
@HUMORCE
Copy link
Member

HUMORCE commented Apr 22, 2022

update:

❯ scoop update git
git: 2.35.3.windows.1 -> 2.36.0.windows.1
Updating one outdated app:
Updating 'git' (2.35.3.windows.1 -> 2.36.0.windows.1)
Downloading new version
Loading PortableGit-2.36.0-64-bit.7z.exe from cache
Checking hash of PortableGit-2.36.0-64-bit.7z.exe ... ok.
Uninstalling 'git' (2.35.3.windows.1)
Removing shim 'git.shim'.
Removing shim 'git.exe'.
Removing shim 'gitk.shim'.
Removing shim 'gitk.exe'.
Removing shim 'git-gui.shim'.
Removing shim 'git-gui.exe'.
Removing shim 'tig.shim'.
Removing shim 'tig.exe'.
Removing shim 'git-bash.shim'.
Removing shim 'git-bash.exe'.
Unlinking ~\scoop\apps\git\current
Installing 'git' (2.36.0.windows.1) [64bit]
Loading PortableGit-2.36.0-64-bit.7z.exe from cache
Extracting dl.7z ... done.
Linking ~\scoop\apps\git\current => ~\scoop\apps\git\2.36.0.windows.1
Creating shim for 'git'.
Creating shim for 'gitk'.
Creating shim for 'git-gui'.
Creating shim for 'tig'.
Creating shim for 'git-bash'.
Creating shortcut for Git Bash (git-bash.exe)
Creating shortcut for Git GUI (git-gui.exe)
ConvertToPrettyJson: C:\Users\HUMOR\scoop\apps\scoop\current\lib\manifest.ps1:48
Line |
  48 |      $file_content = $info | ConvertToPrettyJson
     |                              ~~~~~~~~~~~~~~~~~~~
     | The term 'ConvertToPrettyJson' is not recognized as a name of a cmdlet, function, script file, or
     | executable program. Check the spelling of the name, or if a path was included, verify that the path is
     | correct and try again.

MethodInvocationException: C:\Users\HUMOR\scoop\apps\scoop\current\lib\manifest.ps1:49
Line |
  49 |      [System.IO.File]::WriteAllLines("$dir\install.json", $file_conten|      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Exception calling "WriteAllLines" with "2" argument(s): "Value cannot be null. (Parameter 'contents')"

'git' (2.36.0.windows.1) was installed successfully!
Notes
-----
Set Git Credential Manager Core by running: "git config --global credential.helper manager-core"

fresh install:

❯ scoop install git
Installing 'git' (2.36.0.windows.1) [64bit]
Loading PortableGit-2.36.0-64-bit.7z.exe from cache
Checking hash of PortableGit-2.36.0-64-bit.7z.exe ... ok.
Extracting dl.7z ... done.
Linking ~\scoop\apps\git\current => ~\scoop\apps\git\2.36.0.windows.1
Creating shim for 'git'.
Creating shim for 'gitk'.
Creating shim for 'git-gui'.
Creating shim for 'tig'.
Creating shim for 'git-bash'.
Creating shortcut for Git Bash (git-bash.exe)
Creating shortcut for Git GUI (git-gui.exe)
'git' (2.36.0.windows.1) was installed successfully!
Notes
-----
Set Git Credential Manager Core by running: "git config --global credential.helper manager-core"

@lewis-yeung
Copy link
Contributor

Got the same error after this update.

@niheaven
Copy link
Member Author

Update and try again @HUMORCE @lewis-yeung

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