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

refactor: use ffprobe even if located under /usr/bin #995

Closed
wants to merge 1 commit into from

Conversation

dotlambda
Copy link

@dotlambda dotlambda commented Feb 22, 2023

Summary

If I call _probe_codec_native(source, "/bin/ffmpeg") I want it to use /bin/ffprobe.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
    • I have run task pyright and fixed the relevant issues.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@ooliver1
Copy link
Member

Generally you have user installed binaries in /usr/bin. I'm not against this change but the reason is a bit odd, shouldn't that be in your $PATH?

@dotlambda
Copy link
Author

Generally you have user installed binaries in /usr/bin.

True, I just wanted to shorten the commit message.

I'm not against this change but the reason is a bit odd, shouldn't that be in your $PATH?

Not necessarily. I might e.g. want to test it with an ffmpeg executable I compiled locally to test a change in ffmpeg.

@@ -557,7 +557,7 @@ async def probe(
def _probe_codec_native(
source, executable: str = "ffmpeg"
) -> Tuple[Optional[str], Optional[int]]:
exe = executable[:2] + "probe" if executable in ("ffmpeg", "avconv") else executable
exe = executable[:-4] + "probe" if executable.endswith(("ffmpeg", "avconv")) else executable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a general question, shouldn't proper path manipulation be used for this?
Path(executable).parent / "ffprobe" or similar

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I think

Suggested change
exe = executable[:-4] + "probe" if executable.endswith(("ffmpeg", "avconv")) else executable
exe = Path(executable).parent / "ffprobe" if Path(executable).name in ("ffmpeg", "avconv") else executable

makes the most sense.

@EmreTech EmreTech changed the title use ffprobe even if located under /bin refactor: use ffprobe even if located under /usr/bin Feb 27, 2023
@Skelmis
Copy link
Collaborator

Skelmis commented Apr 9, 2023

Hey @dotlambda, just bumping this. How are you feeling regarding the requested review comments?

@ooliver1
Copy link
Member

Not necessarily. I might e.g. want to test it with an ffmpeg executable I compiled locally to test a change in ffmpeg.

That would then be /usr/local/bin, the order of your PATH variable would determine which binary is used.

@EmreTech EmreTech added p: low Priority: low - not important to be worked on s: awaiting review Status: the issue or PR is awaiting reviews t: refactor Type: refactor - this is a code change but does not fix a bug/add features labels Jun 14, 2023
@Skelmis
Copy link
Collaborator

Skelmis commented Nov 3, 2023

I do not necessarily agree with this for the sake of a change which may only benefit a small subset of users. Can you provide some further reasoning why the library should do this as a default versus you as a developer simply setting an item in the PATH with a higher precedence to be used?

Also some context around how the change actually works given it flips the string substringing order would be appreciated

@ooliver1
Copy link
Member

Closed for the above reasons with no response.

@ooliver1 ooliver1 closed this Mar 23, 2024
@ooliver1 ooliver1 removed p: low Priority: low - not important to be worked on s: awaiting review Status: the issue or PR is awaiting reviews labels Mar 23, 2024
@dotlambda
Copy link
Author

Can you provide some further reasoning why the library should do this as a default versus you as a developer simply setting an item in the PATH with a higher precedence to be used?

My specific use case is in Nixpkgs where I want to set a default ffmpeg executable that's used by any user of nextcord. I can't set PATH for those users because they import a Python module rather than run an executable.
So what I'm trying to achieve is the ability to set executable to an absolute path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: refactor Type: refactor - this is a code change but does not fix a bug/add features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants