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

feature: Prepare extraction of main bucket #3060

Merged
merged 33 commits into from
Apr 24, 2019

Conversation

Ash258
Copy link
Contributor

@Ash258 Ash258 commented Feb 1, 2019

#2939, #2609, #1328

❗❗❗ This PR is not meant to be merged. ❗❗❗

I want to only proof, that extraction of bucket is possible and pretty easy. Bucket folder will not be removed from this PR (to keep diff cleaner, without millions of lines deleted due to bucket folder deletion). If collaborators (This should be done by @r15ch13, since he has access to Excavator) decided to adopt this change, just follow steps bellow (co-authored-by tag would be nice for all commits / PR 😍):

How to?

Slighlty edited and step by step flow from #1328 (comment)

  1. Postpone excavator
  2. Extract bucket folder with history
    • git filter-branch --prune-empty --subdirectory-filter bucket
      • Do it inside normal terminal (not vscode)
        • There will be lots of changes and vscode could be unstable
      • It takes quite a while
      • For me it was ~50 minutes
      • Time
    • git remote set-url origin 'https://github.com/ScoopInstaller/scoop-main'
    • git push -u origin
    • New bucket repository is DONE
      • Sure, there is need to be setuped readme, appveyor, format, binaries, contributing and all other bucket repository things
  3. Create new branch (bucket-removal-adoption)
    • All changes to reflect this change
    • Maybe even PR for some discussion❓
  4. buckets.json
    • Add main bucket into known buckets
  5. Fix code
    1. Install.ps1
      • Download main bucket while installing
        • Since git, 7zip are needed
        • Any better way?
      • checkup should look for added bucket
      • scoop update
        • Current users could not even notice change
        • While updating, check for bucket
          • If there is not added, add it
    2. General code
    3. binaries default dir parameter
  6. Move all package requests (not core related) issues into new repository
  7. Delete bucket folder from core
  8. Run beautifier / vscode format document on all files inside core repository
    1. Use UseCorrectCasing rule
  9. Make master protected
  10. Create develop branch
    • Notify users, who want to test earlier changes
      • scoop config SCOOP_BRANCH develop
        • There is need little fix for this.
        • When you change it, you need to reset to that branch.
          • No it's needed to do it manually using command line.
        • Basicly if git branch -ne get-config SCOOP_BRANCH { git checkout get-config SCOOP_BRANCH }
      • Create issue, about how to use develop branch and what does it involve
      • Pin that issue
    • Never push to master directly
      • All changes will happen in develop branch
      • When change is done, debugged and fully working, PR will be created to master with description like Version: X.XX.XX Adopt new change
  11. Change excavator config with new repository
  12. Start excavator again
  13. ❓ Rename this bucket into Core or something
  14. ❓ Move it to same organization
  15. ❓ Tag and release version 1.0
    • Scoop could adopt normal release process with release tab and changelog inside it.
  16. 🎉🎉 Welcome new era of Scoop with easier maintaining and expanding 🎉🎉

@Ash258
Copy link
Contributor Author

Ash258 commented Feb 1, 2019

Updating works even after this change.

If there is no bucket property inside install.json, 'main' is considered. So updates will be OK, without needing to reinstall.

Only inside scoop list will be [main] instead of empty collumn.

* Maybe add check in list to ignore [main]? Keep it "compatible" https://github.com/lukesampson/scoop/blob/e1bb1e91d0027e00874d06c1fb5966cdae27b994/libexec/scoop-list.ps1#L32

if ($install_info.bucket -eq 'main') {
     Write-Host ''
}

@Ash258
Copy link
Contributor Author

Ash258 commented Feb 1, 2019

How to test?

  1. docker-compose up --build -d; docker-compose run scoop
  2. $env:SCOOP = 'SOME NEW PATH TO TEST'
  3. PS: bin> .\install.ps1
  4. Rename-Item "$env:SCOOP\apps\scoop\current\bucket" "new-bucket"
    • To test updates after extraction, skip this command, revert bucketsdir function and first install some manifests from bucket folder inside core
      +$bucket = relpath '..\bucket' # main bucket
      -$bucket = "$bucketsdir\main" # main bucket
    • scoop install sudo
    • Rename bucket to new-bucket
    • scoop update sudo -f
  5. Test installing, uninstall, whatever

@Ash258
Copy link
Contributor Author

Ash258 commented Feb 1, 2019

I haven't touched on tests yet. But after recent changes (@r15ch13, 589303f), it should be fine.

There is possible to remove default parameter value from https://github.com/lukesampson/scoop/blob/1a845b17ed8f8feb84fdd1052b2e164e8134f50b/test/Scoop-Manifest.Tests.ps1#L1

@Ash258
Copy link
Contributor Author

Ash258 commented Feb 1, 2019

Feel free to suggest any change.

/fyi @r15ch13 @rasa @h404bi @ScoopInstaller

@chawyehsu
Copy link
Member

There is one thing VERY important that you didn't cover above. We need to handle those previously installed packages so that users can still update these packages after this breaking change.

packages installed from the extras bucket has a bucket mark.
install.json

{
    "bucket": "extras",
    "architecture": "64bit"
}

but packages installed from core bucket doesn't.
install.json

{
    "architecture": "64bit"
}

@Ash258
Copy link
Contributor Author

Ash258 commented Feb 1, 2019

It's already handled :)

2f16f0d

@Ash258
Copy link
Contributor Author

Ash258 commented Feb 1, 2019

#3060 (comment)

@niheaven
Copy link
Member

niheaven commented Feb 1, 2019

Like brew? Coooool!

@Ash258
Copy link
Contributor Author

Ash258 commented Feb 1, 2019

Forgot to mention, that this will make #2121 pretty easy to fix, since there will be option to use main/manifest syntax and scoop will no longer rely on alphabet evaluation (based on my testing, that my pwsh from Ash258 bucket is installed always).

So if there will be multiple manifests, just warn user about prefix with desired bucket and exit.

@chawyehsu
Copy link
Member

chawyehsu commented Feb 1, 2019

If a user installs scoop from the first time, the installer script downloads a zip of the core bucket (this is a normal logic, to make scoop work at the first time, git and 7zip are needed). When the user runs scoop update for the first time, they will be notified to install git and 7zip (from the 'zip core bucket').

But the 'zip core bucket' is not a git repository after the installation. So there should be a handler logic to clone a new core bucket and remove the zip one in scoop-update.ps1.

sidenote: I'm working on the brand new installer script which is totally independent and does not rely on any function in core.ps1 and etc, and it provides more configurable install options. The new installer script does the same thing in downloading core bucket.

@Ash258
Copy link
Contributor Author

Ash258 commented Feb 1, 2019

Eveything work as expected. On installing, there is no need to have 'git' repository.

If you just unpack zip to buckets\main, shim scoop and run scoop install git you are good.

See

but i see problem on updating, I hoped, that, .git check for scoop update is applied for buckets too.

Here come question how to handle it? First thing to appear on my mind:

$loc = bucketdir $_ # Line 106
if ($_ -eq 'main' -and !(Test-Path "$loc\.git")) { # main bucket exists, but it's not repo
    rm_bucket 'main'
    add_bucket 'main'
}
Push-Location ($loc)

https://github.com/lukesampson/scoop/blob/2f16f0d1370b9cc50534fd3407a949e2e8377f27/libexec/scoop-update.ps1#L105-L108

@chawyehsu
Copy link
Member

@Ash258 make sense.

@r15ch13 r15ch13 added enhancement review-needed Asks for review of these changes work-in-progress labels Feb 1, 2019
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.

Is there need to change bin\*.ps1's $Dir param? main bucket should has its own bin dir, and manifest related bin should move there, isn't it?

I'll do a deep review tomorrow, in 16 hrs.

bin/auto-pr.ps1 Outdated Show resolved Hide resolved
bin/install.ps1 Outdated Show resolved Hide resolved
bin/install.ps1 Outdated Show resolved Hide resolved
bin/install.ps1 Outdated Show resolved Hide resolved
@@ -141,15 +139,6 @@ function find_hash_in_headers([String] $url) {
function get_hash_for_app([String] $app, $config, [String] $version, [String] $url, [Hashtable] $substitutions) {
$hash = $null

<#
TODO implement more hashing types
Copy link
Member

Choose a reason for hiding this comment

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

I suggest leave hash mode comment for illustrations, there is something TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All are already implemented

Copy link
Member

Choose a reason for hiding this comment

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

I mean TODO more, and leave these implemented alone as comment. Maybe need to rearrange these lines.

@Ash258
Copy link
Contributor Author

Ash258 commented Apr 23, 2019

Made $Dir Parameter inside binaries mandatory and defaults are gone.

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.

LGTM

@Ash258
Copy link
Contributor Author

Ash258 commented Apr 24, 2019

I think this is ready to be merged into develop branch. We can test it today / tomorrow in develop branch and then merge it to master

@r15ch13
Copy link
Member

r15ch13 commented Apr 24, 2019

@Ash258 can you squash some of the commits? Or should I squash the whole PR?

@Ash258
Copy link
Contributor Author

Ash258 commented Apr 24, 2019

Squash whole PR.

@Ash258
Copy link
Contributor Author

Ash258 commented Apr 26, 2019

Looks good after fix for switch branches ❤

A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement review-needed Asks for review of these changes work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants