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

Enabled shimming of external applications #2072

Merged

Conversation

excitoon
Copy link
Contributor

@excitoon excitoon commented Mar 1, 2018

Hi.

Now one can make shims for external applications (system, other applications installed by scoop and so on).

First, $bin is to be searched in $dir, then globally, then in user and system PATH.

Examples:

    "bin": [ [ "winver.exe", "myver", "" ] ]
    "depends": [ "python" ],
    "bin": [
        [ "python.exe", "myscript", "$dir/myscript.py" ],
        [ "python.exe", "myscript.py", "$dir/myscript.py" ]
    ]
    "depends": [ "bash" ],
    "bin": [
        [ "bash.exe", "myscript", "$dir/myscript.sh" ],
        [ "bash.exe", "myscript.sh", "$dir/myscript.sh" ]
    ]

@r15ch13
Copy link
Member

r15ch13 commented Mar 1, 2018

Do you have an example for which this could be useful?

@excitoon
Copy link
Contributor Author

excitoon commented Mar 1, 2018

It could be useful for virtually any case where script is being shipped without interpreter. For example, openssh ships with few Bash scripts (findssl.sh and ssh-copy-id) and it forces maintainer to include bash and its dependencies into manifest (and to file system of every user).
More examples (Python, Perl, Bash):

Of course one can make support of every scripting language (and every version, yep) 'natively' in shims but that seems to be questionable complication from my point of view.

@excitoon
Copy link
Contributor Author

excitoon commented Mar 2, 2018

Basically my point is that every programmer who writes scripts and wants to ship it to Windows systems needs to make some cmd wrapper around them in order to run them from command line. This patch will allow run these scripts in Windows in the same way as in Unix.

@gbursson
Copy link
Contributor

gbursson commented Mar 5, 2018

excitoon, just to let you know, two links (for wakeonlan and swaddle) are incorrect

@stkb
Copy link
Contributor

stkb commented Mar 9, 2018

I've also found this patch useful for a tool I've been using (which is actually a php script).

lib/install.ps1 Outdated
$bin = "$dir\$target"
if(!(is_in_dir $dir $bin)) {
abort "Error in manifest: bin '$target' is outside the app directory."
if(test-path "$dir\$target") {
Copy link
Contributor

Choose a reason for hiding this comment

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

test-path should probably have -pathType leaf to be safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Actually I did not use Powershell before.

lib/install.ps1 Outdated
abort "Error in manifest: bin '$target' is outside the app directory."
if(test-path "$dir\$target") {
$bin = "$dir\$target"
} elseif(test-path $target) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this check is for (if $target is absolute?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, just because absolute path is better than invalid path.

lib/install.ps1 Outdated
} elseif(test-path $target) {
$bin = $target
} else {
$bin = search_in_path $target
}
if(!(test-path $bin)) { abort "Can't shim '$target': File doesn't exist."}
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be something like if(!$bin) { ... } instead. Currently, if we don't find the file, $bin is null and this check blows up, causing lots of errors as the null value propogates.
Also there's no need to do the test-path again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right.

@excitoon
Copy link
Contributor Author

excitoon commented Mar 10, 2018

Rebased first commit onto current master.

@excitoon excitoon changed the title Enabled shimming of external applications WIP Enabled shimming of external applications Mar 10, 2018
@excitoon
Copy link
Contributor Author

excitoon commented Mar 10, 2018

One more thing about For-Each and piping:

    $path.split(';') | % {
        if(test-path "$_\$target" -pathType leaf) {
            return "$_\$target"
        }
    }

If you write it like that, function will return a list consisting of joined return values from each element. That's totally counter-intuitive. If one replace that with regular foreach everything will work as expected.

@excitoon excitoon changed the title WIP Enabled shimming of external applications Enabled shimming of external applications Mar 10, 2018
@excitoon
Copy link
Contributor Author

excitoon commented Mar 10, 2018

I guess it is ready to merge/review again. Tested all mentioned cases.

@rasa rasa added the review-needed Asks for review of these changes label Mar 12, 2018
@rasa rasa requested review from rasa and removed request for rasa March 12, 2018 20:00
Copy link
Member

@rasa rasa left a comment

Choose a reason for hiding this comment

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

LGTM (but untested)

@excitoon
Copy link
Contributor Author

excitoon commented Apr 6, 2018

Hi @r15ch13. Can you merge it (and the other 3 pull requests: #2079, #2118, #2120)?

@r15ch13 r15ch13 merged commit 3e14f37 into ScoopInstaller:master May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-needed Asks for review of these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants