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(chore): Improve git.exe execution and add parallel bucket updates #5122

Merged
merged 13 commits into from
Nov 23, 2022

Conversation

r15ch13
Copy link
Member

@r15ch13 r15ch13 commented Aug 23, 2022

Description

  • By using the direct path to $appdir\git\current\mingw64\bin\git.exe this prevents spawning subprocesses and improves execution time
    • This removes calling two shims. $scoopdir\shims\git.exe calls $appdir\git\current\bin\git.exe which is also a shim.
  • Fixes bucket add fails behind proxy #1615 by replacing currentuser@ with :@
  • Spawns cmd.exe only if the PROXY config value is set and required by the git command
  • Uses PS7 feature Foreach-Object -Parallel to run bucket updates in parallel to improve execution time
    • Not sure how to show the changelog right now, so I just added [extras] behind the commit hash
  • Fallback to sequential updates for PS5
  • Improve git log output

Using a fixed git.exe could be a problem if someone uses git not installed via scoop because this will not detect it with Get-Command anymore.

Motivation and Context

  1. Improve updating with many buckets
  2. Fixes bucket add fails behind proxy #1615

How Has This Been Tested?

  • Locally tested

Checklist:

  • 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.

Before:
image
image

After:
image
image

@r15ch13 r15ch13 changed the base branch from master to develop August 23, 2022 23:40
lib/core.ps1 Outdated Show resolved Hide resolved
@rasa
Copy link
Member

rasa commented Aug 24, 2022

This will almost certainly fix #3877 "Git spawns thousands of times".

@rasa
Copy link
Member

rasa commented Aug 24, 2022

Using a fixed git.exe could be a problem if someone uses git not installed via scoop because this will not detect it with Get-Command anymore.

@r15ch13 Couldn't it test to see which git is first in the path, and only implement this logic, if the first git found is scoop's shim?

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.

libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
libexec/scoop-update.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
@r15ch13
Copy link
Member Author

r15ch13 commented Aug 26, 2022

I tried using Invoke-ExternalCommand() but the redirecting of stdout breaks the output of git. E.g.:

image

Used Invoke-Git() code:

function Invoke-Git {
    [CmdletBinding()]
    [OutputType([String])]
    param(
        [Parameter(Mandatory = $true, Position = 0)]
        [Alias('PSPath', 'Path')]
        [ValidateNotNullOrEmpty()]
        [String]
        $WorkingDirectory,
        [Parameter(Mandatory = $true, Position = 1)]
        [Alias('Args')]
        [String[]]
        $ArgumentList
    )

    $proxy = get_config PROXY
    $git = Get-HelperPath -Helper Git

    $EnvironmentVariables = @{}
    if($proxy -and $proxy -ne 'none')  {
        if(($ArgumentList -join ' ') -Match "\b(clone|checkout|pull|fetch|ls-remote)\b") {
            # convert proxy setting for git
            if ($proxy.StartsWith('currentuser@')) {
                $proxy = $proxy.Replace("currentuser@",":@")
            }
            $EnvironmentVariables.Add('HTTPS_PROXY', $proxy)
            $EnvironmentVariables.Add('HTTP_PROXY', $proxy)
        }
    }

    Invoke-ExternalCommand -FilePath $git -WorkingDirectory $WorkingDirectory -ArgumentList $ArgumentList -EnvironmentVariables $EnvironmentVariables
}

@niheaven
Copy link
Member

niheaven commented Aug 27, 2022

True is the output of Invoke-ExternalCommand and should be omitted by Out-Null.

And for the colorful output, a --color switch must be explicitly added to git's argument (can be done in Invoke-Git). But for --oneline, the ref inside bracket are missing (i.e., (HEAD -> master, origin/master, origin/HEAD))

PS, this is related to Invoke-Git and may be done after the PR if it needs additional tweaks (to keep consistence with former output).

@rashil2000
Copy link
Member

Yes, the --color flag should be added

@r15ch13
Copy link
Member Author

r15ch13 commented Aug 28, 2022

And --decorate shows the missing (HEAD -> master, origin/master, origin/HEAD).

But iec doesn't support redirecting the output to a variable which is required for getting the git commands output.

E.g.

$previousCommit = Invoke-Git -Path $currentdir -ArgumentList @('rev-parse', 'HEAD')
$currentRepo = Invoke-Git -Path $currentdir -ArgumentList @('config', 'remote.origin.url')
$currentBranch = Invoke-Git -Path $currentdir -ArgumentList 'branch'

@rashil2000
Copy link
Member

I think we can skip on using iec for Invoke-Git for now. When iec becomes more robust, we can switch to using it.

@r15ch13
Copy link
Member Author

r15ch13 commented Aug 28, 2022

Uses try-catch-finally to revert the environment variables.

Unified the output for both bucket update methods to:
image

@niheaven
Copy link
Member

niheaven commented Aug 29, 2022

Is the @() necessary for ArgumentList? If you use comma (,), it is an [Array] for the argument already.

And I agreed that iec implementation of Invoke-Git() could be in another PR, after some refactoring of iec (Why not we called it Start-ExternalProcess()? I found it just a rewrite of Start-Process Cmdlet with more features 😄 )

r15ch13 and others added 6 commits November 12, 2022 16:58
- By using direct path to ~\scoop\apps\git\current\mingw64\bin\git.exe this prevents spawning subprocesses and improves execution time
- Fixes ScoopInstaller#1615 by replacing currentuser@ with :@
- Spawns cmd.exe only if proxy is set and required by the git command
- Use PS7 feature Foreach-Object -Parallel to run bucket updates in parallel to improve execution time
- Fallback to sequential updates for PS5
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
@r15ch13
Copy link
Member Author

r15ch13 commented Nov 12, 2022

Just a rebase onto the current dev branch.

@r15ch13
Copy link
Member Author

r15ch13 commented Nov 21, 2022

Abbreviated the hash to 12 hex digits to prevent inconsistent output length.

Ready for review 😄

@niheaven
Copy link
Member

It works for me, and two questions here:

  1. Sync-Bucket is only used by Sync-Buckets once, so why not copy its code in Sync-Buckets and rename latter to Sync-Bucket?
  2. Buckets name and updated time are separated by space, should updated time be aligned?
    if ($PSVersionTable.PSVersion.Major -ge 7) {
        # Parallel parameter is available since PowerShell 7
        $buckets | Where-Object { $_.valid } | ForEach-Object -ThrottleLimit 5 -Parallel {
            . "$using:PSScriptRoot\..\lib\core.ps1"

            $bucketLoc = $_.path
            $name = $_.name

            $previousCommit = Invoke-Git -Path $bucketLoc -ArgumentList @('rev-parse', 'HEAD')
            Invoke-Git -Path $bucketLoc -ArgumentList @('pull', '-q')
            if ($using:Log) {
                Invoke-Git -Path $bucketLoc -ArgumentList @('--no-pager', 'log', '--color', '--no-decorate', "--grep='^(chore)'", '--invert-grep', '--abbrev=12', "--format='tformat: * %C(yellow)%h%Creset %<|(72,trunc)%s %Cgreen$name%Creset %C(cyan)%cr%Creset'", "$previousCommit..HEAD")
            }
        }
    } else {
        $buckets | Where-Object { $_.valid } | ForEach-Object {
            $bucketLoc = $_.path
            $name = $_.name

            $previousCommit = Invoke-Git -Path $bucketLoc -ArgumentList @('rev-parse', 'HEAD')
            Invoke-Git -Path $bucketLoc -ArgumentList @('pull', '-q')
            if ($Log) {
                Invoke-Git -Path $bucketLoc -ArgumentList @('--no-pager', 'log', '--color', '--no-decorate', "--grep='^(chore)'", '--invert-grep', '--abbrev=12', "--format='tformat: * %C(yellow)%h%Creset %<|(72,trunc)%s %Cgreen$name%Creset %C(cyan)%cr%Creset'", "$previousCommit..HEAD")
            }
        }
    }

image

@r15ch13
Copy link
Member Author

r15ch13 commented Nov 22, 2022

The idea for Sync-Bucket was that it just works with one bucket. For later use:tm: :smile: (whatever that may be)
And I couldn't get the parallel foreach to work with a function. So I guess merging it into Sync-Buckets is okay. 😄

Didn't want to truncate the bucket name for the alignment. But it could determine the longest name and align it.

@r15ch13
Copy link
Member Author

r15ch13 commented Nov 22, 2022

Yes, but it doesn't update one bucket. So the name is misleading. I know PSScriptAnalyzer doesn't like it 😄
Any other name suggestions?

@niheaven
Copy link
Member

niheaven commented Nov 22, 2022

It could update one bucket, right? Think about Rename-Item or Remove-Item, PS always uses single word for these multiple item(s) manipulating functions. So Sync-Bucket is fine IMO.

@niheaven
Copy link
Member

LGTM!

## [Unreleased](https://github.com/ScoopInstaller/Scoop/compare/master...develop)

And the changelog? (headline above)

@rashil2000
Copy link
Member

new output: image

This output is quite wide, IMO. Default terminal emulator widths are like 30-40 columns. May I suggest outputting a PSObject list for this? (similar to scoop list)

We will lose the individual colors, but PowerShell will automatically take care of padding, alignment and terminal adjustment. (Not to mention the output will become parseable by PowerShell. Can't think of a use case for now, but who knows 😅)

@r15ch13
Copy link
Member Author

r15ch13 commented Nov 22, 2022

image
Fits in both default terminals of Windows 11

This is just output for the user. If someone wants parse this it would be better to use Git directly on the buckets itself 😄

@niheaven niheaven changed the title Improve git.exe execution and add parallel bucket updates feat(chore): Improve git.exe execution and add parallel bucket updates Nov 22, 2022
@niheaven niheaven merged commit 360daa7 into ScoopInstaller:develop Nov 23, 2022
@niheaven
Copy link
Member

@r15ch13 Occasional the pull process will be outputted, why?

image

@r15ch13
Copy link
Member Author

r15ch13 commented Dec 17, 2022

hm, I don't know 🤷🏽
Never happend while testing this

@niheaven
Copy link
Member

Tend to be appeared after a long time pull. (and maybe a poor network?)

Have seen it several times when I don't scoop update for several days.

Change LAST_UPDATE and reset bucket to weeks or months ago and run scoop update? (I'll change it and test)

@niheaven
Copy link
Member

Wow, it is.

image

I've reset my buckets repo to 3 weeks ago (one or two is enough) and run scoop update, the red output appears again.

@r15ch13
Copy link
Member Author

r15ch13 commented Dec 17, 2022

How did you reset the repo? I tried a hard reset to an older commit but it doesn't hit the network to change back to the latest.

@rashil2000
Copy link
Member

Also the scoop bucket subcommands seem to be broken after this PR:
image

@r15ch13
Copy link
Member Author

r15ch13 commented Dec 18, 2022

Can't reproduce both issues. 😕

Output of git --version?
image

@rashil2000
Copy link
Member

git version 2.39.0.windows.1

@niheaven
Copy link
Member

image

For specific git version?

@niheaven
Copy link
Member

@r15ch13 If git reset gives Updating files: 100% (513/513), done. Then scoop update will output the red Updating files too.

image

@HUMORCE
Copy link
Member

HUMORCE commented Dec 18, 2022

screenshot_20221218153543
screenshot_20221218153548

cannot reproduce, both Powershell 7.2 and 5.1.

git version 2.39.0.windows.1

@rashil2000
Copy link
Member

My Scoop home is located at a non default directory, with a space in the path. Maybe that's the reason?

@niheaven
Copy link
Member

See the Updating files prompt, @HUMORCE you haven't met them, so the output is fine.

For @rashil2000's bug, I have a non-default location of scoop w/o space, and cannot reproduce. Maybe it is the space?

@rashil2000
Copy link
Member

Looks like all commands related to Git have an issue when Scoop home is a spaced path.

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

Successfully merging this pull request may close these issues.

bucket add fails behind proxy
5 participants