feat: add support for windows wsl2 #1190

Vergenter · 2023-08-07T03:21:01Z

Pull Request Template

Type of change

Bug fix
Feature
Documentation update

Description

This update provides a simpler method for Windows users to use the application when utilizing WSL2.
A current issue exists with syncplay. It does not work correctly. Simple way to reproduce it is to run it from WSL2 using:

"/mnt/c/Program Files (x86)/Syncplay/syncplay.exe"

It silently fails if a URL is specified(even during runtime), causing the window to close unexpectedly. A workaround for this is: (not currently applied)

cmd.exe /C "C:\Program Files (x86)\Syncplay\syncplay.exe" "$URL"

Moreover, using uname -s to switch cases for syncplay is insufficient and requires modification.

-s | --syncplay)
            case "$(uname -s)" in
                Darwin*) player_function="/Applications/Syncplay.app/Contents/MacOS/syncplay" ;;
                MINGW* | *Msys)
                    export PATH="$PATH":"/c/Program Files (x86)/Syncplay/"
                    player_function="syncplay.exe"
                    ;;
                *) case "$(uname -r)" in
                    *WSL2*)
                        export PATH="$PATH:/mnt/c/Program Files (x86)/Syncplay/"
                        player_function="syncplay.exe"
                        ;;
                    *)
                        player_function="syncplay"
                        ;;
                esac

                ;; 
            esac
            ;;

The checks I've confirmed manually are already marked. Other checks, not yet verified, should function correctly given that only the display program is being modified. I will complete the rest after resolving the syncplay issue.

Checklist

any anime playing
bumped version

-h help info is up to date
Readme is up to date
Man page is up to date

Additional Testcases

The safe bet: One Piece
Episode 0: Saenai Heroine no Sodatekata ♭
Unicode: Saenai Heroine no Sodatekata ♭
Non-whole episodes: Tensei shitara slime datta ken (ep. 24.5, ep. 24.9)

port19x · 2023-08-07T10:07:59Z

I'm skeptical if we should explicitly support this in a documented fashion.
Your changes to the ani-cli matching are welcome tho.

As you see some automated checks failed, clean those up and I'll do a more thorough review

justchokingaround · 2023-08-07T12:14:22Z

no need to add that to the README

port19x

Yeah, readme is fat enough as is

README.md

Co-authored-by: port19 <port19@port19.xyz>

Vergenter · 2023-08-07T20:41:42Z

Long story short:

`syncplay` treats links as files

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "syncplayClient.py", line 17, in <module>
  File "syncplay\ep_client.pyc", line 8, in main
  File "syncplay\clientManager.pyc", line 9, in run
  File "syncplay\ui\ConfigurationGetter.pyc", line 557, in getConfiguration
  File "syncplay\ui\ConfigurationGetter.pyc", line 471, in _loadRelativeConfiguration
  File "syncplay\ui\ConfigurationGetter.pyc", line 462, in __getRelativeConfigLocations
  File "ntpath.pyc", line 651, in realpath
  File "ntpath.pyc", line 601, in _getfinalpathname_nonstrict
FileNotFoundError: [WinError 161] The specified path is invalid: '\\\\wsl$\\Ubuntu\\home\\vergenter\\ani-cli\\https:\\www054.vipanicdn.net\\streamhls\\f134bc36b9bb59de2ef28a48a7cef6bf\\ep.5.1683732036.1080.m3u8'

The only workaround I found for this is to call it using cmd.exe (PowerShell also has a problem with the url-path).
cmd.exe /C "syncplay.exe"

cmd call and argument(title) with spaces

I cannot use cmd in a way described below because then I cannot handle spaces in the title. WSL loses the ability to use the absolute path when escaping spaces in the title in any way.

vergenter@DESKTOP-JDQ0A50:/mnt/c$ cmd.exe /C "C:\Program Files (x86)\Syncplay\syncplay.exe" "123" --  --force-media-title="123 123"
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.

Without a space in the title it works fine...

Solutions

Use the path for syncplay, but it needs to be the Windows path rather than the WSL path, as cmd won't use the WSL paths. This is rejected because Windows syncplay does not add itself to the path.
cd to the path. This is ugly, but it avoids all problems with the title. Additionally, checking dependencies for syncplay this way works correctly.

It should be now ready to review.

Should I modify this part?
https://github.com/Vergenter/ani-cli/blob/ad2b2c00072612d4c4dbd203dc3a6536b37e6cbc/ani-cli#L346-L364

port19x

I'd rather not have such large code addition for an unsupported plattform.
We support windows natively.
The expanded matching for a wsl2 uname is welcome, but that's about the extend I'm willing to make concessions for wsl users.

ani-cli

Vergenter · 2023-08-08T13:30:43Z

I'd rather not have such large code addition for an unsupported plattform. We support windows natively. The expanded matching for a wsl2 uname is welcome, but that's about the extend I'm willing to make concessions for wsl users.

Understood. I worked on adding syncplay support for WSL2 because it's on the required list.

However, note that currently syncplay is supported in weird/hacky way:

For some reason, it switches using a different uname.
Syncplay doesn't officially support URLs; it uses file paths as arguments.

justchokingaround · 2023-08-08T13:31:47Z

syncplay works with URLs, it just doesn't support m3u8 links

Vergenter · 2023-08-08T14:05:33Z

syncplay works with URLs, it just doesn't support m3u8 links

vergenter@DESKTOP-JDQ0A50:~$ syncplay --help
usage: syncplay [-h] [--no-gui] [-a hostname] [-n username] [-d] [-g] [--no-store] [-r [room]] [-p [password]]
                [--player-path path] [--language language] [--clear-gui-data] [-v]
                [--load-playlist-from-file loadPlaylistFromFile]
                [file] [options ...]

Solution to synchronize playback of multiple media player instances over the network.

positional arguments:
  file                  file to play
  options               player options, if you need to pass options starting with - prepend them with single '--'
                        argument

First positional argument is a file and this is how it is used in ani-cli.

ani-cli/ani-cli

Line 258 in d4f9c22

    
           *yncpla*) nohup "$player_function" "$episode" -- --force-media-title="${allanime_title}Episode ${ep_no}" >/dev/null 2>&1 & ;;

Using url in place of file is undocumented behaviour and it fails because of this in WSL.

BTW. Code is ready I applied change proposals.

justchokingaround · 2023-08-08T14:11:18Z

Using url in place of file is undocumented behaviour and it fails because of this in WSL.

wym? the "file" that is passed in ani-cli is literally a url. ik it because i implemented it. i'm telling you that links that end with .m3u8 do not work with syncplay, no matter which platform.

have you tested in wsl syncplay with .mp4 links?

ani-cli

Vergenter · 2023-08-08T14:20:23Z

wym? the "file" that is passed in ani-cli is literally a url. ik it because i implemented it. i'm telling you that links that end with .m3u8 do not work with syncplay, no matter which platform.

have you tested in wsl syncplay with .mp4 links?

As I previously mentioned WSL2 fails, because of it:
That is from syncplay logs(URL is parsed like path):

FileNotFoundError: [WinError 161] The specified path is invalid: '\\\\wsl$\\Ubuntu\\home\\vergenter\\ani-cli\\https:\\www054.vipanicdn.net\\streamhls\\f134bc36b9bb59de2ef28a48a7cef6bf\\ep.5.1683732036.1080.m3u8'

justchokingaround · 2023-08-08T14:25:48Z

does the same thing happen with mp4 files?

Vergenter · 2023-08-08T14:47:11Z

does the same thing happen with mp4 files?

If by mp4 you mean for example this:

Then yes.

FileNotFoundError: [WinError 161] The specified path is invalid: '\\\\wsl$\\Ubuntu\\home\\vergenter\\ani-cli\\https:\\myanime.sharepoint.com\\sites\\chartlousty\\_layouts\\15'

This is filled correctly:

wsl prefix is not visible there:

If I start syncplay from windows and manually paste url it works ok.
If I start syncplay from wsl and manually paste url it fails.

justchokingaround · 2023-08-08T14:50:34Z

does syncplay even work with any links that are not local files, in wsl?

justchokingaround · 2023-08-08T14:50:40Z

even youtube links

Vergenter · 2023-08-08T14:56:06Z

even youtube links

All URL fails using syncplay from wsl.(m3u8 from windows syncplay work ok, I just tested)

FileNotFoundError: [WinError 161] The specified path is invalid: '\\\\wsl$\\Ubuntu\\home\\vergenter\\ani-cli\\https:\\www.youtube.com'

justchokingaround · 2023-08-08T14:58:20Z

oh ok, then we ignore syncplay support for wsl

Vergenter · 2023-08-08T15:08:59Z

@justchokingaround
How does in other situation url work at all?
This is how it is handled in syncplay:
https://github.com/Syncplay/syncplay/blob/6b490a40980bd61307ea20ede37fd7e4d2c8073e/syncplay/ui/ConfigurationGetter.py#L556-L557
It is def _loadRelativeConfiguration(self): that handles it
https://github.com/Syncplay/syncplay/blob/6b490a40980bd61307ea20ede37fd7e4d2c8073e/syncplay/ui/ConfigurationGetter.py#L460-L480
I don't undestand how any URL survives that.

justchokingaround · 2023-08-08T15:10:37Z

@port19x can you check this please, i haven't used syncplay in a while, and i don't have it installed atm

port19x · 2023-08-09T09:12:16Z

@port19x can you check this please, i haven't used syncplay in a while, and i don't have it installed atm

I can once I'm home, but better open a separate issue / ping me on discord because I'll probably forget until the end of the workday

Vergenter added 2 commits August 7, 2023 02:59

add support for windows wsl2

13a128b

add support for vlc (WSL)

7848745

Vergenter requested review from justchokingaround and port19x as code owners August 7, 2023 03:21

fix formating and bump version

552a384

Vergenter changed the title ~~Add support for windows wsl2~~ feat: Add support for windows wsl2 Aug 7, 2023

Vergenter changed the title ~~feat: Add support for windows wsl2~~ feat: add support for windows wsl2 Aug 7, 2023

Vergenter added 2 commits August 7, 2023 13:33

bump version to 4.5.6

6d2defb

Merge remote-tracking branch 'upstream/master'

3001b04

port19x requested changes Aug 7, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

port19x mentioned this pull request Aug 7, 2023

refactor: remove -N flag in favor of -S + -e detection #1193

Merged

17 tasks

Vergenter and others added 3 commits August 7, 2023 21:58

add support for syncplay(wsl)

398470e

remove Windows WSL2 from README.md

c06ba65

Co-authored-by: port19 <port19@port19.xyz>

Merge branch 'pystardust:master' into master

ad2b2c0

port19x requested changes Aug 8, 2023

View reviewed changes

ani-cli Outdated Show resolved Hide resolved

ani-cli Outdated Show resolved Hide resolved

remove workaround for wsl2 syncplay

c23ba5b

justchokingaround reviewed Aug 8, 2023

View reviewed changes

ani-cli Show resolved Hide resolved

justchokingaround approved these changes Aug 8, 2023

View reviewed changes

port19x approved these changes Aug 9, 2023

View reviewed changes

port19x merged commit ed9d149 into pystardust:master Aug 9, 2023
6 checks passed

Vergenter mentioned this pull request Feb 5, 2024

Asks for Windows dependencies when inside WSL 2 #1266

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for windows wsl2 #1190

feat: add support for windows wsl2 #1190

Vergenter commented Aug 7, 2023 •

edited

port19x commented Aug 7, 2023

justchokingaround commented Aug 7, 2023

port19x left a comment

Vergenter commented Aug 7, 2023 •

edited

port19x left a comment

Vergenter commented Aug 8, 2023

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023 •

edited

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023 •

edited

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023 •

edited

justchokingaround commented Aug 8, 2023

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023

justchokingaround commented Aug 8, 2023

port19x commented Aug 9, 2023

feat: add support for windows wsl2 #1190

feat: add support for windows wsl2 #1190

Conversation

Vergenter commented Aug 7, 2023 • edited

Pull Request Template

Type of change

Description

Checklist

Additional Testcases

port19x commented Aug 7, 2023

justchokingaround commented Aug 7, 2023

port19x left a comment

Choose a reason for hiding this comment

Vergenter commented Aug 7, 2023 • edited

Long story short:

syncplay treats links as files

cmd call and argument(title) with spaces

Solutions

It should be now ready to review.

port19x left a comment

Choose a reason for hiding this comment

Vergenter commented Aug 8, 2023

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023 • edited

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023 • edited

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023 • edited

justchokingaround commented Aug 8, 2023

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023

justchokingaround commented Aug 8, 2023

Vergenter commented Aug 8, 2023

justchokingaround commented Aug 8, 2023

port19x commented Aug 9, 2023

Vergenter commented Aug 7, 2023 •

edited

Vergenter commented Aug 7, 2023 •

edited

`syncplay` treats links as files

Vergenter commented Aug 8, 2023 •

edited

Vergenter commented Aug 8, 2023 •

edited

Vergenter commented Aug 8, 2023 •

edited