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

Add support for resetting of apps to be imported #5572

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

xxthunder
Copy link

@xxthunder xxthunder commented Jul 9, 2023

Description

Added a command line option for scoop import:

  • -r, --reset: reset each app of imported JSON after installation

Motivation and Context

I use the import command quite often to define apps and their versions as dependencies in my projects. The reason for this is of course reproducibility of builds. But when different versions from different buckets of an app are already installed, it might still happen that the wrong apps and/or versions are used.
Closes #5525

How Has This Been Tested?

As there are no unit tests available for the changes file (actually there are no functions in scoop-import.ps1) I tested the changes exploratorily with my local scoopfile.json's.
My changes do affect the scoop import feature only.

Checklist:

  • I have read the Contributing Guide.
  • 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.

Comment on lines +70 to +66
if ($reset) {
& "$PSScriptRoot\scoop-reset.ps1" $app
}
Copy link
Member

Choose a reason for hiding this comment

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

It should not be necessary, because juntion/shims/shorts/etc. of apps is point the latest version when update app.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe it should not, but it is. As I described in the issue #5525 if install different versions of the same app, no install or update command gives you what you want.

Example: I have the apps python310 and python311 out of the versions bucket installed. python311 was the last one and so the shim python.exe points to that of python311. If I now import a scoopfile.json refering to python310, nothing is changed during import. I end up with the wrong python.exe in the PATH. Therefore the reset argument.

Copy link
Member

Choose a reason for hiding this comment

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

This happens after both python310 and python311 have been installed already, then the import operation is performed right?

Copy link
Author

Choose a reason for hiding this comment

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

Exactly.

Copy link
Member

Choose a reason for hiding this comment

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

@xxthunder technically, this is also not required. To see what I mean:

Scoop/lib/install.ps1

Lines 58 to 66 in 6cdcc75

create_shims $manifest $dir $global $architecture
create_startmenu_shortcuts $manifest $dir $global $architecture
install_psmodule $manifest $dir $global
env_add_path $manifest $dir $global $architecture
env_set $manifest $dir $global $architecture
# persist data
persist_data $manifest $original_dir $persist_dir
persist_permission $manifest $global

The functions that scoop-reset will run (see below) have already been run by scoop-install (see above).

create_shims $manifest $dir $global $architecture
create_startmenu_shortcuts $manifest $dir $global $architecture
env_add_path $manifest $dir $global $architecture
env_set $manifest $dir $global $architecture
# unlink all potential old link before re-persisting
unlink_persist_data $manifest $original_dir
persist_data $manifest $original_dir $persist_dir
persist_permission $manifest $global

Copy link
Author

Choose a reason for hiding this comment

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

@rashil2000 @HUMORCE I think this is not true. I want the reset no matter what is already installed. So if I import with, e.g., cmake version 3.24 in my scoopfile.json and cmake 3.24 and 3.26 both already installed and p3.26 was installed the last time. Then the shim cmake points to cmake 3.26. Now I can import my scoopfile as often as I want, nothing will be installed because 3.24 is already installed and nothing will be resetted. So the shim cmake points to cmake 3.26. Exactly this my problem with scoop import. My option --reset solves this.

libexec/scoop-import.ps1 Outdated Show resolved Hide resolved
@xxthunder xxthunder force-pushed the 5525-import-support-reset-update branch from 6fab401 to 2811956 Compare July 25, 2023 15:46
@xxthunder xxthunder requested a review from HUMORCE July 25, 2023 15:48
libexec/scoop-import.ps1 Outdated Show resolved Hide resolved
@rashil2000
Copy link
Member

@xxthunder I don't think the update flag is required at all. There are basically three types of apps in the imported scoopfile:

    $app = if ($item.Source -in $bucket_names) {
        "$($item.Source)/$($item.Name)"
    } elseif ($item.Source -eq '<auto-generated>') {
        "$($item.Name)@$($item.Version)"
    } else {
        $item.Source
    }
  1. The first case corresponds to apps of the form <bucket>/<app>. When this case is present, always the latest version of it will be installed.
  2. The second case corresponds to a fixed/pinned version. Updates should not be carried out in this case.
  3. The third case is similar to the second - it points either to a remote URL or a local filepath, where the manifest has a fixed version.

Basically, only those apps, which belong to a particular bucket, are eligible for updates. And this is covered in the (1) scenario (because buckets are always up-to-date, as Scoop follows the rolling release model of updates).

@xxthunder xxthunder force-pushed the 5525-import-support-reset-update branch 2 times, most recently from 8ee47fb to 2fa0e4f Compare October 16, 2023 18:04
@xxthunder
Copy link
Author

@rashil2000 you are right, removed the update flag again. Added two small fixes in import and install. Please have a look when you have time.

@xxthunder xxthunder force-pushed the 5525-import-support-reset-update branch from 2fa0e4f to 68c8ae1 Compare October 16, 2023 18:06
@xxthunder xxthunder force-pushed the 5525-import-support-reset-update branch from 68c8ae1 to adce1ed Compare October 19, 2023 12:17
@xxthunder xxthunder changed the title Add support for resetting and updating of apps to be imported Add support for resetting of apps to be imported Oct 19, 2023
@rashil2000
Copy link
Member

Check my comment on HUMORCE's thread.

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