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

Create sh shim #1951

Merged
merged 7 commits into from
Feb 19, 2018
Merged

Create sh shim #1951

merged 7 commits into from
Feb 19, 2018

Conversation

Fleshgrinder
Copy link
Contributor

Closes #1949

lib/install.ps1 Outdated
@@ -701,7 +701,7 @@ function rm_shim($name, $shimdir) {
}

# other shim types might be present
'.exe', '.shim', '.cmd' | % {
'', ''.exe', '.shim', '.cmd' | % {
Copy link
Contributor

Choose a reason for hiding this comment

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

Syntax error - there's an extra ' before the .exe

@stkb
Copy link
Contributor

stkb commented Feb 10, 2018

I would like to see something like this implemented too, and after correcting the syntax error it works with the (limited) testing I've one. However I think the shellscript shims should only be created if the target is a .ps1 or a .cmd/.bat file. For the .exe's they're just redundant.

@Fleshgrinder
Copy link
Contributor Author

@stkb fixed the syntax error. I agree and it's what I wrote in #1951 too. We could simply return from the exe block, what do you think?

@stkb
Copy link
Contributor

stkb commented Feb 11, 2018

@Fleshgrinder 👍 I would have put the shellscript shim creation in both the .bat and the .ps1 blocks instead, even if it's a bit of repetition, but your suggestion works just as well. I don't really have a preference.

@stkb
Copy link
Contributor

stkb commented Feb 11, 2018

Wait sorry I just noticed something, we do need separate shim code for the .bat/.cmd and the .ps1 paths, otherwise we get this for .cmd shims:

#!/bin/sh
powershell -ex unrestricted "path/to/gvim.cmd" "$@"

Now starting powershell to run a .cmd file does work, but it's not really the best, especially given how slow PS is starting up :)

Just this is apparently enough for .cmd shims

#!/bin/sh
"path/to/cmd_or_bat.cmd" "$@"

Testing this with Git Bash btw, I don't know if any of this stuff works in WSL or Cygwin

@Fleshgrinder
Copy link
Contributor Author

Your proposed CMD shim cannot work in WSL because CMD files have no shebang, hence, Linux won't be able to figure out what to start it with. However, we can simply use cmd "path/to/script.bat" "$@" in this case which always works. Will update the PR in a bit.

lib/core.ps1 Outdated
@@ -234,6 +234,9 @@ function shim($path, $global, $name, $arg) {
# shim .bat, .cmd so they can be used by programs with no awareness of PSH
$shim_cmd = "$(strip_ext($shim)).cmd"
"@`"$(resolve-path $path)`" $arg %*" | out-file $shim_cmd -encoding ascii

$shim_sh = "$(strip_ext($shim))"
"#!/bin/sh`ncmd `"$(resolve-path $path)`" `"$@`"" | out-file $shim_sh -encoding ascii
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't $arg be in here too?
"#!/bin/sh`ncmd `"$(resolve-path $path)`" $arg `"$@`""

lib/core.ps1 Outdated
$shim_cmd = $(strip_ext($shim))
"#!/bin/sh`npowershell -ex unrestricted `"$(resolve-path $path)`" `"$@`"" | out-file $shim_cmd -encoding ascii
$shim_sh = "$(strip_ext($shim))"
"#!/bin/sh`npowershell -ex unrestricted `"$(resolve-path $path)`" `"$@`"" | out-file $shim_sh -encoding ascii
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too
"#!/bin/sh`npowershell -ex unrestricted `"$(resolve-path $path)`" $arg `"$@`""

@stkb
Copy link
Contributor

stkb commented Feb 14, 2018

Nice. We just forgot to put in $arg there for the shim args I think (haven't tested my suggestions)

@Fleshgrinder
Copy link
Contributor Author

Only question left is if we have to quote $arg? Is this always a single thing or is it actually $args and multiple things?

@stkb
Copy link
Contributor

stkb commented Feb 14, 2018

It's not a very good name IMO, but it's either a single string or an array of strings.*
They're taken straight from the manifest, where they should already be quoted if necessary I guess. So I don't think it's necessary to quote them. Existing code doesn't either

* There's some code that treats $arg as possibly an array, but from some quick testing I'm not sure it can be. Will look into that after this PR.

@stkb
Copy link
Contributor

stkb commented Feb 14, 2018

Yeah, $arg is always a string (of args joined with spaces), never an array. So it doesn't need to be joined/quoted. The .ps1 shim code that tries to do that leads to a bug. But that's a separate issue.

@stkb
Copy link
Contributor

stkb commented Feb 14, 2018

Ok one more thing: I don't know about WSL but just cmd "path/to/script" isn't working in Git Bash. It needs the /C switch. After changing line 239 to this it's all working for me: (note the double //)

        "#!/bin/sh`ncmd //C `"$(resolve-path $path)`" $arg `"$@`"" | out-file $shim_sh -encoding ascii

@Fleshgrinder
Copy link
Contributor Author

I added the switch for script execution and immediate termination. I guess we're fine now. 👍

@stkb
Copy link
Contributor

stkb commented Feb 15, 2018

👍 (cc @r15ch13)

@r15ch13
Copy link
Member

r15ch13 commented Feb 16, 2018

Some more changes:

  • remove excessive strip_ext calls
  • remove excessive resolve-path calls
  • add .jar support (see selenium/digdag)
function shim($path, $global, $name, $arg) {
    if(!(test-path $path)) { abort "Can't shim '$(fname $path)': couldn't find '$path'." }
    $abs_shimdir = ensure (shimdir $global)
    if(!$name) { $name = strip_ext (fname $path) }

    $shim = "$abs_shimdir\$($name.tolower())"

    # convert to relative path
    pushd $abs_shimdir
    $relative_path = resolve-path -relative $path
    popd
    $resolved_path = resolve-path $path

    # if $path points to another drive resolve-path prepends .\ which could break shims
    if($relative_path -match "^(.\\[\w]:).*$") {
        write-output "`$path = `"$path`"" | out-file "$shim.ps1" -encoding utf8
    } else {
        write-output "`$path = join-path `"`$psscriptroot`" `"$relative_path`"" | out-file "$shim.ps1" -encoding utf8
    }
    if($arg) {
        write-output "`$args = '$($arg -join "', '")', `$args" | out-file "$shim.ps1" -encoding utf8 -append
    }
    if($path -match '\.jar$') {
        "if(`$myinvocation.expectingInput) { `$input | & java -jar `$path @args } else { & java -jar `$path @args }" | out-file "$shim.ps1" -encoding utf8 -append
    } else {
        "if(`$myinvocation.expectingInput) { `$input | & `$path @args } else { & `$path @args }" | out-file "$shim.ps1" -encoding utf8 -append
    }

    if($path -match '\.exe$') {
        # for programs with no awareness of any shell
        cp "$(versiondir 'scoop' 'current')\supporting\shimexe\shim.exe" "$shim.exe" -force
        write-output "path = $resolved_path" | out-file "$shim.shim" -encoding utf8
        if($arg) {
            write-output "args = $arg" | out-file "$shim.shim" -encoding utf8 -append
        }
    } elseif($path -match '\.((bat)|(cmd))$') {
        # shim .bat, .cmd so they can be used by programs with no awareness of PSH
        "@`"$resolved_path`" $arg %*" | out-file "$shim.cmd" -encoding ascii

        "#!/bin/sh`ncmd //C `"$resolved_path`" $arg `"$@`"" | out-file $shim -encoding ascii
    } elseif($path -match '\.ps1$') {
        # make ps1 accessible from cmd.exe
"@echo off
setlocal enabledelayedexpansion
set args=%*
:: replace problem characters in arguments
set args=%args:`"='%
set args=%args:(=``(%
set args=%args:)=``)%
set invalid=`"='
if !args! == !invalid! ( set args= )
powershell -noprofile -ex unrestricted `"& '$resolved_path' %args%;exit `$lastexitcode`"" | out-file "$shim.cmd" -encoding ascii

        "#!/bin/sh`npowershell -ex unrestricted `"$resolved_path`" $arg `"$@`"" | out-file $shim -encoding ascii
    } elseif($path -match '\.jar$') {
        "@java -jar `"$resolved_path`" $arg %*" | out-file "$shim.cmd" -encoding ascii
        "#!/bin/sh`njava -jar `"$resolved_path`" $arg `"$@`"" | out-file $shim -encoding ascii
    }
}

@r15ch13
Copy link
Member

r15ch13 commented Feb 16, 2018

Add -PathType leaf to test-path because if $name and $_ are empty it will delete everything from the shims directory. This can happen when there is no shim name in the manifest (killed my scoop installation like that, which lead to 7176719 😁)

    "bin": [
        ["main.exe", "", "-somearg"]
    ],
    # other shim types might be present
    '', '.exe', '.shim', '.cmd' | % {
        if(test-path -Path "$shimdir\$name$_" -PathType leaf ) {
            rm "$shimdir\$name$_"
        }
    }

@Fleshgrinder
Copy link
Contributor Author

Fleshgrinder commented Feb 19, 2018

@r15ch13 two questions:

  1. Is it a good idea to provide the JAR support with this PR?
  2. Why would we wrap the JAR execution in a PowerShell script on SH? We can directly call Java and skip PowerShell.

@r15ch13
Copy link
Member

r15ch13 commented Feb 19, 2018

@Fleshgrinder yeah, there are some programs that only provide a *.jar and everyone of them has a custom post_install property that just creates a ps1 script (see my examples above)
The code above creates a bash script for .jar file (see last lines 😄)

@Fleshgrinder
Copy link
Contributor Author

Oh, missed the last condition and only looked at the first jar check, all good, commit coming up. Many thanks for your great help here.

@r15ch13 r15ch13 merged commit b885ecf into ScoopInstaller:master Feb 19, 2018
@Fleshgrinder Fleshgrinder deleted the sh-term-support branch February 19, 2018 19:49
This was referenced Nov 5, 2018
r15ch13 pushed a commit that referenced this pull request Nov 5, 2018
Fixes #2740.
Adding .exe suffix to cmd is needed in WSL.
Removing MSYS' slash-escaping from //C is needed in WSL, Cygwin, or anywhere outside MSYS (or Git Bash). Doesn't break MSYS after .exe suffix is added.

See also #1949, #1951
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