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.

Sign up for GitHub

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

Jump to bottom

mpv(-git): Add to path instead of shimming #9662

Merged
merged 1 commit into from
Dec 1, 2022
Merged

mpv(-git): Add to path instead of shimming #9662

merged 1 commit into from
Dec 1, 2022

Conversation

sitiom
Copy link
Collaborator

@sitiom sitiom commented Nov 6, 2022 •
edited
Loading

mpv.net fails to terminate MPV subprocesses when using the shim. This was causing problems such as thumbfast not displaying the thumbnails properly. Adding mpv directly to the path fixes the problem.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2022

Your changes do not pass checks.

mpv-git

  • Description
  • License
  • Hashes
  • Checkver
  • Autoupdate
  • Autoupdate Hash Extraction

@sitiom

This comment was marked as outdated.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2022

Your changes do not pass checks.

mpv-git

  • Description
  • License
  • Hashes
  • Checkver
  • Autoupdate
  • Autoupdate Hash Extraction

@sitiom
Copy link
Collaborator Author

sitiom commented Nov 6, 2022 •
edited
Loading

  • Autoupdate Hash Extraction

@sitiom sitiom changed the title mpv-git: Add to path instead of shimming mpv-git: Add to path instead of shimming & Use sourceforge checkver Nov 6, 2022
@sitiom sitiom mentioned this pull request Nov 6, 2022
Closed
1 task
@sitiom

This comment was marked as outdated.

Sign in to view
@sitiom sitiom changed the title mpv-git: Add to path instead of shimming & Use sourceforge checkver mpv(-git): Add to path instead of shimming, Use sourceforge checkver, Remove updater.bat Nov 6, 2022
@sitiom

This comment was marked as outdated.

Sign in to view
sitiom added a commit to LGUG2Z/komorebi-application-specific-configuration that referenced this pull request Nov 9, 2022
@sitiom
chore(mpv): remove rule
5bbac1b 
Turns out mpv ghost windows are a problem of scoop shimming, fix in
ScoopInstaller/Extras#9662
@sitiom

This comment was marked as outdated.

Sign in to view
@github-actions
Copy link
Contributor

Your changes do not pass checks.

mpv-git

  • Description
  • License
  • Hashes
  • Checkver
  • Autoupdate

mpv

  • Description
  • License
  • Hashes
  • Checkver
  • Autoupdate

@sitiom

This comment was marked as outdated.

Sign in to view
2 similar comments
@sitiom

This comment was marked as outdated.

Sign in to view
@sitiom
Copy link
Collaborator Author

sitiom commented Nov 29, 2022

/verify

@sitiom
Copy link
Collaborator Author

sitiom commented Nov 29, 2022

@niheaven Something wrong with the verifier?

@niheaven
Copy link
Member

niheaven commented Nov 29, 2022 •
edited
Loading

https://github.com/ScoopInstaller/Extras/actions/runs/3572539543

I've started it again.

@sitiom
Copy link
Collaborator Author

sitiom commented Nov 29, 2022

ScoopInstaller/Extras/actions/runs/3572539543

It has just started, and the former one is blocked maybe by SF?

It got skipped though? And the rerun also failed

@niheaven
Copy link
Member

SF's some error, please wait for a moment?

@sitiom
Copy link
Collaborator Author

sitiom commented Nov 29, 2022

SF's some error, please wait for a moment?

It's been like that since I opened this PR though, I'm not sure if that helps.

@niheaven
Copy link
Member

See the first attempt:

INFO: Finished bucket/mpv-git.json checks

INFO: Starting bucket/mpv.json checks

mpv-git is fine, while mpv failed. So it should be SF's error.

@niheaven
Copy link
Member

It's mpv's checkver, please change to:

    "checkver": {
        "sourceforge": "mpv-player-windows/release",
        "regex": "mpv-([\\d.]+)-"
    },

@sitiom

This comment was marked as outdated.

Sign in to view
@niheaven
Copy link
Member

Now it's SF's, wait...

@sitiom

This comment was marked as outdated.

Sign in to view
1 similar comment
@sitiom
Copy link
Collaborator Author

sitiom commented Dec 1, 2022

/verify

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

All changes look good.

Wait for review from human collaborators.

mpv-git

  • Description
  • License
  • Hashes
  • Checkver
  • Autoupdate

mpv

  • Description
  • License
  • Hashes
  • Checkver
  • Autoupdate

@sitiom
Copy link
Collaborator Author

sitiom commented Dec 1, 2022

@niheaven all good

@niheaven niheaven changed the title mpv(-git): Add to path instead of shimming, Use sourceforge checkver, Remove updater.bat mpv(-git): Add to path instead of shimming Dec 1, 2022
@niheaven niheaven merged commit 8f71579 into ScoopInstaller:master Dec 1, 2022
@sitiom sitiom deleted the patch-1 branch December 1, 2022 02:27
@sitiom sitiom mentioned this pull request Dec 1, 2022
Merged
1 task
@DanSM-5 DanSM-5 mentioned this pull request Jan 11, 2023
Closed
3 tasks
@rashil2000
Copy link
Member

mpv.net fails to terminate MPV subprocesses when using the shim. This was causing problems such as thumbfast not displaying the thumbnails properly. Adding mpv directly to the path fixes the problem.

Wait, mpv.net is a separate manifest: https://github.com/ScoopInstaller/Extras/blob/master/bucket/mpv.net.json, so why is the change required here?

@sitiom
Copy link
Collaborator Author

sitiom commented Feb 27, 2023

Mpv GUIs make use of the mpv executable to display thumbnails (po5/thumbfast#47 (comment)). The shim was causing issues with properly terminating the mpv subprocess and was causing issues like a black screen.

@rashil2000
Copy link
Member

Is this issue happening with mpv or with mpv.net?

@sitiom
Copy link
Collaborator Author

sitiom commented Feb 27, 2023 •
edited
Loading

For mpv.net and other mpv GUIs, yes. Eventually, the ghost processes will pile up and will hog resources over time if you just ignore it.

@rashil2000
Copy link
Member

Sorry, I'm still confused.

mpv.net does not use or depend on mpv, so how will changing mpv fix the issue?

@sitiom
Copy link
Collaborator Author

sitiom commented Mar 5, 2023

The thumbfast plugin depends on the mpv executable as stated in #9662 (comment). The gist is that scripts calling mpv in a subprocess will not work properly.

@jack-mil jack-mil mentioned this pull request Dec 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niheaven niheaven niheaven approved these changes

Assignees
No one assigned
Labels
review-needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sitiom @niheaven @rashil2000