[PATCH] gnu: obs: Update obs to fb347c.

This version of obs adds support for OBS_PLUGINS*_PATH environment variables,

which are required to be able to load plugins, which are present in profile.

It will make it possible for following packages to work:

---

gnu/packages/video.scm | 116 ++++++++++++++++++++++-------------------

1 file changed, 63 insertions(+), 53 deletions(-)

It's a commit from master branch containing changes from PR:

It's just few commits after 26.1.2 and it seems there is no any big

changes since that and should be safe to use.

I asked for 26.1.3 tag, but it seems that it won't happen in nearest

future: https://github.com/obsproject/obs-studio/issues/4100

As one of developers said to me, usually obs releases once in 3-4

months, that is why I decided to use a commit instead of tag.

The changes is needed to be able to specify plugin load path using

native-search-paths to be able to package obs plugins with guix.

On Tue, Jan 26, 2021 at 06:59:39PM +0300, Andrew Tropin wrote:

Thanks!

Your patch doesn't apply to the current master branch. Can you rebase it

and send a revision?

Hi Adrew,

I patched obs here [1]. I checked the upstream patch, and noticed that it ammends AddExtraModulePaths function. With Guix the list of plugin directories will contain *two* locations with the very same plugins, one with the obs store and guix-profile, hence obs will print 'Duplicate library?' warning messages.

I packaged the main location to avoid such a message.

Footnotes:

[1] https://issues.guix.gnu.org/45707

--

Alexey

It's very true, obs, will have same plugins in load paths twice, but

from what I found the problem will happen only during shutdown of obs,

when it will try to unload the same plugin twice, which doesn't affect

runtime anyhow. I had a small workaround for that (a separate

envirnoment variable, which toggles the loading of plugins from

INSTALL_PREFIX), but during review one of obs mantainers said that it's

an adhoc solution and better to solve this problem in general. The

comment from @kkartaltepe and related changes [fn:1].

The other solution was to check if OBS_PLUGINS_PATH is present and ommit

loading of builtin plugins in that case, but it will break in case

someone wants to specify just additional plugin load path (some non-guix

use case).

I decided to go without any fix to that problem as it is not directly

related to changeset I proposed and doesn't affect runtime, but it's

probably a good idea to report double free on shutdown issue to

upstream.

[fn:1] https://github.com/obsproject/obs-studio/pull/4067

This version of obs adds support for OBS_PLUGINS*_PATH environment variables,

which are required to be able to load plugins, which are present in profile.

It will make it possible for following packages to work:

---

gnu/packages/video.scm | 116 ++++++++++++++++++++++-------------------

1 file changed, 63 insertions(+), 53 deletions(-)

On Wed, Jan 27, 2021 at 12:45:30PM +0300, Andrew Tropin wrote:

Thanks for this!

I'm wondering, instead of building from a Git commit, can we cherry-pick

the commit as a patch?

Does it work if we apply that to the 26.1.2 release?

It would be a little more precise.

Hello Leo!

It should work, originally I wrote a patch for 26.1.2 and later rebased

the commit on master without any conflicts.

There was a discussion on irc about it and at the end of the day I would

prefer to use the commit from master branch, rather than maintaining a

separate patch, which we will need to remove later, when 26.1.3 or

another version will be released.

There were not so many changes since 26.1.2 up to the fb347c, I fluently

went through them, they are mostly minor and were accepted after the

pretty strict code review process.

From what I know it should be safe to use this commit, but I can contact

upstream devs and additionally clarify that.

Overall I'm ok with using an exact patch, but prefer using a commit from

master. WDYT?

--

Best regards,

Andrew Tropin

On Tue, Feb 02, 2021 at 02:39:03PM +0300, Andrew Tropin wrote:

Thanks for explaining the process you went through. Based on that, I

agree that your patch is good as proposed.

The patch file doesn't apply to current master. It looks like some other

changes regarding this plugins problem have been done to the obs package

since your changes were made.

Can you rebase it on your end and re-generate the patch? Or is it still

necessary?

I'll post to the related thread first https://issues.guix.gnu.org/45707

to discuss what we can do about that situation, and will post back here,

when we will reach some agreement.

--

Best regards,

Andrew Tropin

Hi,

Andrew Tropin <andrew@trop.in> skribis:

Ah ha! So on ‘master’, there’s the OBS_PLUGINS_DIRECTORY patch that you

provided earlier, which is why I wrote that the plugins should work.

Should we wait for the next OBS release instead of packaging an

arbitrary commit?

Ludo’.

Hi,

Thanks for merging my patch =) I am not insist on keeping [1], but I do think that is more cleaner solution.

With the upstream patch, Obs will be able to extend the list of plugin directories. We are going to add ~/.guix-profile/ to that list, which means Obs will have two different locations with partially different so files. It doesn't look clean to me.

In addition, [1] doesn't require to have OBS_PLUGINS_DIRECTORY variables. If there is no such, it will run with its defaults.

Footnotes:

[1] https://issues.guix.gnu.org/45707

--

Alexey

I'm not in a hurry, but as I explained earlier in this thread [fn:2] it seems

relatively safe for me to use this commit and getting back later to

26.1.3 or 26.2.0 or whatever next release will be.

In terms of implememntation I like that [fn:1] prevents double loading

of plugins, by excluding obs installation dir from "load-path". However

double loading of the same plugin doesn't seem to break anything. Also,

OBS_PLUGINS_DIRECTORY variable name maybe a little better than

OBS_PLUGINS_PATH as it contains only one path.

The problem is that now there are two almost identical mechanisms (one

in upstream and one via patch [fn:1]), which can bring some maintanance

problems in the future.

There are two good option in my opinion:

- contribute patch from [fn:1] to upstream (reverting OBS_PLUGINS_PATH)

- revert [fn:1] and use OBS_PLUGINS_PATH from upstream

If Alexey ready to contibute OBS_PLUGINS_DIRECTORY patch to obs

(reverting OBS_PLUGINS_PATH), I would be glad to support it. Otherwise,

I would prefer to revert [fn:1] and apply this one. To prevent

maintanance problems in the future.

[fn:2] http://issues.guix.gnu.org/46113

[fn:1] https://issues.guix.gnu.org/45707

--

Best regards,

Andrew Tropin

Hi!

Andrew Tropin <andrew@trop.in> skribis:

I think it’s nicer to follow upstream in general, but I also don’t mind

using our OBS_PLUGINS_DIRECTORY patch until we upgrade to the next

release.

So, Andrew and Alexey: lemme know and I’ll apply what you consider best!

Ludo’.

Hi Andrew,

Andrew Tropin <andrew@trop.in> writes:

I would go with this one. But the thing is that [fn:1] is specific and

makes sense only for guix. At least from my point of view. So I doubt

that the upstream accept it. But you can try for sure.

As I am not a regular obs user, I am afraid I won't be able to find time

for this soon. Feel free to use/delete/revert [fn:1]. It is your call.

--

Alexey

Thank you for the response!

Probably I won't find time to contribute a patch from [fn:1] to obs any

time soon too. So I will wait for next obs release and will update

package definition to use upstream solution once a new version is

released.

Thank you for your work, sorry for inconvenience)

--

Best regards,

Andrew Tropin