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

decompress.ps1: Refactored (w/ install.ps1, core.ps1) (edited) #3169

Merged
merged 2 commits into from
Mar 7, 2019

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Feb 25, 2019

EDITED: Split PR, PART I

  • Move Functions (to decompress.ps1)
    • extract_zip and unzip_old from core.ps1
    • unpack_inno, extract_msi, lessmsi_config and extract_lessmsi from install.ps1
  • Modify Test
    • Move unzip_old test from Scoop-Core.Tests.ps1 to Scoop-Decompress.Tests.ps1

Thanks for @Ash258 #3149, this could be part of it.

@Ash258
Copy link
Contributor

Ash258 commented Feb 25, 2019

I would rather see 3 PRs for each list item (instead of 1 huge) in small iteration windows. This increase error domain and could be hard to debug. So:

  1. Move functions
    • Review
    • Merge
    • Wait for feedback of users (few days)
    • Repeat for next item

@niheaven niheaven force-pushed the refactor-decompress branch from c86ed93 to b421879 Compare February 26, 2019 03:22
@niheaven niheaven changed the title decompress.ps1: Refactored (w/ install.ps1, core.ps1) decompress.ps1: Refactored (w/ install.ps1, core.ps1) (edited) Feb 26, 2019
@niheaven
Copy link
Member Author

Split PR and this is the one for moving function.

Other fix (rewrite functions, etc.) will be submitted after merged.

@Ash258 @r15ch13 @rasa

@niheaven niheaven force-pushed the refactor-decompress branch from b421879 to b98d8ac Compare February 26, 2019 03:39
@Ash258 Ash258 mentioned this pull request Mar 3, 2019
14 tasks
@r15ch13
Copy link
Member

r15ch13 commented Mar 4, 2019

Working and ready for merge? Can't test right now.

@Ash258
Copy link
Contributor

Ash258 commented Mar 4, 2019

I am not sure if this change do not break installation of new scoop. Since only core is downloaded and extract_zip is called (https://github.com/lukesampson/scoop/blob/master/bin/install.ps1#L49)

@niheaven Have you tested installation?

@Ash258
Copy link
Contributor

Ash258 commented Mar 4, 2019

These "hacks" and continuality thinking about not breaking installation will not be needed when bucket extraction and standalone installer (https://github.com/scoopinstaller/install) is done and is not dependent on scoop's core implementation.

@niheaven
Copy link
Member Author

niheaven commented Mar 4, 2019

@Ash258 @r15ch13 Fully tested with last commit.

Replace extract_zip with .NET's [IO.Compression.ZipFile]::ExtractToDirectory() in bin/install.ps1. Since requirement of Powershell is 3+, Expand-Archive introduced in PS 5 cannot be used (which should be better).

This PR can be tested by the following code:

[environment]::setEnvironmentVariable('SCOOP','D:\TEST_SCOOP_PATH','User')
$env:SCOOP='D:\TEST_SCOOP_PATH\Scoop'
iex (new-object net.webclient).downloadstring('https://raw.githubusercontent.com/niheaven/scoop/test-scoop/bin/install.ps1')
scoop config SCOOP_REPO "https://github.com/niheaven/scoop"
scoop config SCOOP_BRANCH "refactor-decompress"
scoop update
scoop install ag

test-scoop/bin/install.ps1 is a modified install script that redirect to PR's branch, and scoop update, scoop install ag(zip installer) both okay.

I've test above code with Unicode path (测试\Scoop), long path (looooooooooooooooooooooooooooooooooooooooongpaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaath\Scoop)
of 'TEST_SCOOP_PATH', etc., and all passed.

Please remember to restore scoop config and env values after test.

@Ash258
Copy link
Contributor

Ash258 commented Mar 4, 2019

LGTM. By static code review. Will fully test it in few minutes, after i arrive home.

@niheaven
Copy link
Member Author

niheaven commented Mar 4, 2019

The new scoop installer uses the same method that I used here, and Expand-Archive for PS 5+, too.

@niheaven
Copy link
Member Author

niheaven commented Mar 7, 2019

@Ash258 Does the test be passed?

Copy link
Contributor

@Ash258 Ash258 left a comment

Choose a reason for hiding this comment

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

LGTM

@Ash258
Copy link
Contributor

Ash258 commented Mar 7, 2019

Tested basic scoop installation, manifest installation / updating.

@r15ch13 r15ch13 merged commit 5f91682 into ScoopInstaller:master Mar 7, 2019
@niheaven niheaven deleted the refactor-decompress branch March 8, 2019 01:36
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.

3 participants