mintty / wsltty

Mintty as a terminal for Bash on Ubuntu on Windows / WSL
Other
3.11k stars 104 forks source link

Please update the scoop package #304

Closed SonaliBendre closed 2 years ago

mintty commented 2 years ago

Please address the request to the scoop package maintainer.

SonaliBendre commented 2 years ago

Let's maintain our own scoop bucket with mintty apps

mintty commented 2 years ago

Not sure what you mean. There is only one wsltty (i.e. mintty targetted to WSL). So why a distinct bucket? And what's the problem with the "extras" bucket?

SonaliBendre commented 2 years ago

And what's the problem with the "extras" bucket?

https://github.com/mintty/wsltty/issues/304#issuecomment-1046120781

mintty commented 2 years ago

So I interpret your sparse comment so that you think the package is orphaned and you'd like to take over, or make a fresh one. Actually, it might be good to address the issue at the scoop project, to ask their opinion about handling the situation. In either case, feel free to simply make such a package.

rashil2000 commented 2 years ago

Hey @mintty

I have previously taken a jab at fixing the Scoop package, but I failed. You can see my attempts here - https://github.com/ScoopInstaller/Extras/pull/2318

Basically, what we want to achieve is that the install.bat file should install everything inside the specified scoop package folder (denoted by $dir in the manifest). Likewise, the uninstall.bat file should remove stuff from this folder and cleanup things like shortcuts etc.

The user configuration files should also be stored in the same folder (can be in a separate sub-folder, if desired).

Thanks in advance!

mintty commented 2 years ago

You can pass a desired installation target directory as a parameter to the install script, and a desired configuration directory as a second parameter. Apart from that, the error quoted in that issue

Running installer... The batch file cannot be found.

occurs in the scoop wrapper for installation, which I do not know. There have been a few changes for the installer as you can see in git log. If you can identify a commit that made it fail, we can debug further. Please check first whether the latest working scoop package, the current one, would still build and install in the current scoop environment.

rashil2000 commented 2 years ago

Yes, that's what I tried:

    "installer": {
        "file": "install.bat",
        "args": [
            "\"$dir\"",
            "\"$dir\\config\""
        ],
        "keep": true
    },
    "uninstaller": {
        "file": "uninstall.bat"
    },

Will test it once again and let you know. Just one question. Can the batch file be invoked from any directory? Or do we need to cd somewhere first, as done in the pre_install step here?

    "pre_install": [
        "Set-Content \"$dir\\install.bat\" ('@cd %~dp0', (Get-Content \"$dir\\install.bat\")) -Encoding ASCII -Force",
        "Set-Content \"$dir\\uninstall.bat\" ('@cd %~dp0', (Get-Content \"$dir\\uninstall.bat\")) -Encoding ASCII -Force"
    ],
mintty commented 2 years ago

It needs to be run from the directory where the archive was unpacked, to have the local reference to all the files it copies.

rashil2000 commented 2 years ago

I have tried multiple things but I'm still not able to make it work. This is where I am right now:

{
    "version": "3.5.3",
    "description": "Mintty as a terminal for WSL (Windows Subsystem for Linux).",
    "homepage": "https://github.com/mintty/wsltty",
    "license": "GPL-3.0-or-later",
    "architecture": {
        "64bit": {
            "url": "https://github.com/mintty/wsltty/releases/download/3.5.3/wsltty-3.5.3-x86_64.cab#/dl.7z",
            "hash": "977b5e23a9cea8d1f6a5fc74966601ccfd5dd6521b071e18116285f19e08c0b0",
            "extract_dir": "wsltty-3.5.3-x86_64"
        },
        "32bit": {
            "url": "https://github.com/mintty/wsltty/releases/download/3.5.3/wsltty-3.5.3-i686.cab#/dl.7z",
            "hash": "81dc5bfc4bec5dc92c90d14390ff9d2d9d5aeb9f763873f44656e5d39073e699",
            "extract_dir": "wsltty-3.5.3-i686"
        }
    },
    "pre_install": [
        "Set-Content \"$dir\\test.bat\" (@('@echo off', 'echo curr %cd%', 'echo args now:', 'echo %1', 'echo %2'))"
    ],
    "installer": {
        "script": [
            "Push-Location $dir",
            "& \"$dir\\test.bat\" \"$dir\" \"$dir\\config\"",
            "& \"$dir\\install.bat\" \"$dir\" \"$dir\\config\"",
            "Pop-Location"
        ]
    },
    "uninstaller": {
        "script": [
            "Push-Location $dir",
            "& \"$dir\\uninstall.bat\"",
            "Pop-Location"
        ]
    },
    "persist": "config",
    "checkver": "github",
    "autoupdate": {
        "architecture": {
            "64bit": {
                "url": "https://github.com/mintty/wsltty/releases/download/$version/wsltty-$version-x86_64.cab#/dl.7z",
                "extract_dir": "wsltty-$version-x86_64"
            },
            "32bit": {
                "url": "https://github.com/mintty/wsltty/releases/download/$version/wsltty-$version-i686.cab#/dl.7z",
                "extract_dir": "wsltty-$version-i686"
            }
        }
    }
}

I created a test.bat file in pre_install step to echo the current directory and supplied args. Could you please test it and see what's wrong? The command would be scoop install <full path to above JSON file>.

rashil2000 commented 2 years ago

I think there's a bug in install.bat - it doesn't work if the arguments (paths) given to it contain spaces.

mintty commented 2 years ago

Please cross-check with space-less arguments, to narrow down the issue. Trying to debug myself, after a bunch of updates, scoop install C:\tmp\wsltty.scoop (with the contents above) tells me: Couldn't find manifest for 'C'. which I don't understand.

mintty commented 2 years ago

OK, renaming to scoop.json (strange restriction), it says wsltty 3.5.3 was installed. It runs just a second, however, so I have a vague feeling that it does not really work. Trying to debug that, I tried another install which says

WARN  'wsltty' (3.5.3) is already installed.
Use 'scoop update wsltty' to install a new version.

But either update or uninstall claim ERROR 'C:\tmp\wsltty.json' isn't installed. so I'm stuck with further debugging. Seems to be a scoop bug that it doesn't have a clear view of what's installed or not.

mintty commented 2 years ago

OK, after manual removal of %USERPROFILE%\scoop\apps\wsltty I can install again, and it works. Postinstall does not create a context menu short "WSL Terminal" (which I'd suggest to do) but it can be added via the WSLtty start menu entry. So back to the issue, do you have a space in your user profile name?

rashil2000 commented 2 years ago

Yes, I do have a space in the fullpath to \scoop\apps folder.

mintty commented 2 years ago

Spaces in the user name had been fixed before (#230) but apparently not for the case of parameters to install.bat.

rashil2000 commented 2 years ago

Additionally, since you have tested the manifest, does the uninstallation complete successfully? Or does it require the environment variable installdir to be set externally first?

(I was able to test installation by temporarily providing a non-space path, but this did not work for uninstallation).

mintty commented 2 years ago

With the quirky parameter passing mechanism of DOS/Windows scripts ("batch files"), it seems to be impossible to apply quoting properly so that passing space-embedding parameters would work. There is a workaround, however. Please try to set environment variables APPDATA and LOCALAPPDATA before calling install.bat, and do that without parameters.

rashil2000 commented 2 years ago

The issue with setting these variables would be that the files would be installed in subfolders of $dir.

The parent scoop folder is already called 'wsltty', so this can cause confusion among users.

mintty commented 2 years ago

Tweaked the installer, please test: set installdir=... and configdir=... before running install.bat (now without parameters); set installdir=... before running uninstall should also work

rashil2000 commented 2 years ago

Just tried, it ignored the variables (installdir, configdir) specified and installed it to AppData and LocalAppData. I think you need to remove these two lines:

https://github.com/mintty/wsltty/blob/5cee2c341b4a29c17167bfc393f1c907d71540e3/install.bat#L7-L8

rashil2000 commented 2 years ago

If I remove these two lines, there's another problem now. The line removes all the bat files from the current directory and installation fails 🤔

https://github.com/mintty/wsltty/blob/5cee2c341b4a29c17167bfc393f1c907d71540e3/install.bat#L26

mintty commented 2 years ago

I has messed up the previous tweak, now fixed. Works for me now. Do you by any chance call the script within the target installdir? That would not work indeed. The assumption is that the files are unpacked in a temporary directory, then installed to its target, as typical Windows packages do.

rashil2000 commented 2 years ago

Okay, everything is working as intended now.

Do you plan to make another release, or instead edit the current release files to include the patched install.bat?

mintty commented 2 years ago

Next mintty release would be some time this month I think, with some stunning new feature... If you'd like a fix release, though, I'll make one. (Patching an existing release is generally not appreciated.)

rashil2000 commented 2 years ago

No worries, it is up to you. The Scoop mintty package is already functional (it just installs to the default AppData location), so we can wait for the next release. Just ping me on this thread and I will update the manifest.

mintty commented 2 years ago

Released 3.6.0.