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
obs-studio: Add builtin support for browser source #99284
Conversation
Oh damn, I'm late to the party! #99242 I'll update my PR to only add support for the browser source. |
I've updated it now. Should I also remove obs-linuxbrowser, now that it's no longer needed? If so, how do I do that? |
@Munksgaard I would assume you would need to remove nixpkgs/pkgs/top-level/all-packages.nix Line 21026 in 84cf00f
|
@Munksgaard diff --git a/pkgs/applications/video/obs-studio/default.nix b/pkgs/applications/video/obs-studio/default.nix
index 35c7ce93c6d..93eecae8d5c 100644
--- a/pkgs/applications/video/obs-studio/default.nix
+++ b/pkgs/applications/video/obs-studio/default.nix
@@ -80,12 +80,13 @@ in mkDerivation rec {
postUnpack = ''
mkdir -p cef/Release cef/Resources cef/libcef_dll_wrapper/
for i in ${libcef}/share/cef/*; do
- ln -s $i cef/Release/
- ln -s $i cef/Resources/
+ cp -r $i cef/Release/
+ cp -r $i cef/Resources/
done
- ln -s ${libcef}/lib/libcef.so cef/Release/
- ln -s ${libcef}/lib/libcef_dll_wrapper.a cef/libcef_dll_wrapper/
- ln -s ${libcef}/include cef/
+ cp -r ${libcef}/lib/libcef.so cef/Release/
+ cp -r ${libcef}/lib/libcef_dll_wrapper.a cef/libcef_dll_wrapper/
+ cp -r ${libcef}/include cef/
+ ls -R cef
'';
# obs attempts to dlopen libobs-opengl, it fails unless we make sure I don't know if this is a proper fix but this works. I think the underlying issues is that obs-studio's findCEF.cmake is flawed. |
Thank you, I'll try it later tonight.
…On Fri, Oct 2, 2020, at 16:24, Moritz Hedtke wrote:
@Munksgaard <https://github.com/Munksgaard>
Also I fixed the patchelf thing by using the following patch
diff --git a/pkgs/applications/video/obs-studio/default.nix
b/pkgs/applications/video/obs-studio/default.nix
index 35c7ce93c6d..93eecae8d5c 100644
--- a/pkgs/applications/video/obs-studio/default.nix
+++ b/pkgs/applications/video/obs-studio/default.nix
@@ -80,12 +80,13 @@ in mkDerivation rec {
postUnpack = ''
mkdir -p cef/Release cef/Resources cef/libcef_dll_wrapper/
for i in ${libcef}/share/cef/*; do
- ln -s $i cef/Release/
- ln -s $i cef/Resources/
+ cp -r $i cef/Release/
+ cp -r $i cef/Resources/
done
- ln -s ${libcef}/lib/libcef.so cef/Release/
- ln -s ${libcef}/lib/libcef_dll_wrapper.a cef/libcef_dll_wrapper/
- ln -s ${libcef}/include cef/
+ cp -r ${libcef}/lib/libcef.so cef/Release/
+ cp -r ${libcef}/lib/libcef_dll_wrapper.a cef/libcef_dll_wrapper/
+ cp -r ${libcef}/include cef/
+ ls -R cef
'';
# obs attempts to dlopen libobs-opengl, it fails unless we make sure
I don't know if this is a proper fix but this works. I think the
underlying issues is that obs-studio's findCEF.cmake is flawed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#99284 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABYJVK3RD5E7SQPBOHBWITSIXPDVANCNFSM4SARAOGQ>.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it locally - the changes in #99284 (comment) could still be done in my opinion. I don't think a migration of the package is necessary.
Oh yes, I forgot! Fixed that now :) |
Not pushed yet? @Munksgaard |
Sorry! Should be done now :) |
What's needed for this to get merged? |
Rebased on top of current master (to support 26.0.2) |
Can we get this merged? @mohe2015 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise it should be good do go.
I don't have merge permissions btw.
cp -r ${libcef}/lib/libcef.so cef/Release/ | ||
cp -r ${libcef}/lib/libcef_dll_wrapper.a cef/libcef_dll_wrapper/ | ||
cp -r ${libcef}/include cef/ | ||
ls -R cef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should probably be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and rebased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be merged in my opinion. @Munksgaard Where did you get the version of cef from? Because I couldn't find any recommendation at obs-studio's repository and http://opensource.spotify.com/cefbuilds/index.html#linux64 shows that version 86 would be available. Or is cef also used by other packages? Anyways this would be something for a later pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you broke something. My rebuild took a bit longer because I also rebased to master but I can't find the browser source option. Please check yourself.
Edit: Submodule fetching seems to be broken @Munksgaard
If we're at it you could also change the commit messages according to the contributing guide. I think "libcef: oldversion -> 75.1.14" should be better. See https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md
@@ -44,8 +45,9 @@ in mkDerivation rec { | |||
src = fetchFromGitHub { | |||
owner = "obsproject"; | |||
repo = "obs-studio"; | |||
rev = version; | |||
rev = "refs/tags/${version}"; | |||
sha256 = "1d502f80whh686mvq0yn6zpa5nvmnlzxwp5sjz43vpbbvhpbrdqj"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sha256 = "1d502f80whh686mvq0yn6zpa5nvmnlzxwp5sjz43vpbbvhpbrdqj"; | |
sha256 = "vMrPMuZPeFefpbtQykeWifLBkhkSM47tDPid5cU3xa0="; |
something changed the hash and this didn't get attention because we both had the old version in cache probably.
Update libcef package to 75.1.14 This will allow us to enable browser source in the obs-studio build. It will also mean that obs-linuxbrowser probably doesn't work, but it's not needed any longer, so that shouldn't be a problem.
Since version 25, OBS Studio has shipped with a version of obs-browser that includes linux support (which means that we don't need the obs-linuxbrowser any more). This commit enables the builtin browser source. Fixes NixOS#98035 Thanks to @mohe2015 for fixing a patchelf issue I had!
No longer needed since NixOS#99284
I took it from the build instructions on the obs-studio wiki: https://github.com/obsproject/obs-studio/wiki/Install-Instructions#linux-build-directions. Trying to build with cef 86.0.2 causes problems I don't know how to fix:
I've updated the sha now, and it should compile correctly again. Please take a look when you have time @mohe2015 Edit: And thank you, once again, for helping me out! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@puffnfresh How can I push this forward? |
@MP2E, @jb55, @puffnfresh: You guys were automatically assigned as reviewers on this PR, what can I do to push it forward? Is there someone else I should be nudging instead? |
Philip Munksgaard <notifications@github.com> writes:
@MP2E, @jb55, @puffnfresh: You guys were automatically assigned as
reviewers on this PR, what can I do to push it forward? Is there
someone else I should be nudging instead?
I haven't been able to test this yet but it's an awesome plugin that I
would love to see in nixpkgs!
I don't have commit access so I can't do much personally to push it
through
|
Result of 1 package failed to build:
4 packages built:
|
Thanks, tested and LGTM. |
Thank you @mweinelt :) |
Happy to help. |
Motivation for this change
Bump obs-studio and enable builtin support for browser source.
Since version 25, OBS Studio has shipped with a version of obs-browser that includes linux support (which means that we don't need the obs-linuxbrowser any more). However, previously, we didn't built obs-studio with that support enabled. This PR enables it.
For some reason, I'm getting a
patchelf: open: Permission denied
at the end of mynix-build -A obs-studio
, but I can run the binary manually, and I have confirmed that the browser source is present and working. Does anyone have an idea how I can fix the permission issue?This PR removes the need for the obs-linuxbrowser package (obs-linuxbrowser has also been archived and discontinued, as detailed here)
Fixes #98035
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)