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

gnome-subtitles: init at 1.7.2 #95098

Closed
wants to merge 1 commit into from
Closed

gnome-subtitles: init at 1.7.2 #95098

wants to merge 1 commit into from

Conversation

dasj19
Copy link
Contributor

@dasj19 dasj19 commented Aug 10, 2020 •
edited

Motivation for this change

WIP. Adding the gnome-subtitle package to the repo as requested here: #92176

Things done

Now the package builds and runs fine.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dasj19 dasj19 mentioned this pull request Aug 10, 2020
Closed
@dasj19
Copy link
Contributor Author

dasj19 commented Aug 12, 2020

Now I got the application to work ... it opens subtitles and supports video playback.

@danieldk
Copy link
Contributor

You are still doing far too much work that should be done by autoreconfHook and wrapGAppsHook. I made a short example in the direction I meant:

https://gist.github.com/danieldk/acfd76fb510803a8ec68f7a58f6cc053

It compiles and runs, but I didn't test it (it may require more dependencies for all functionality to work). Just meant as an example to show how these hooks work.

@dasj19
Copy link
Contributor Author

dasj19 commented Aug 13, 2020

You are still doing far too much work that should be done by autoreconfHook and wrapGAppsHook. I made a short example in the direction I meant:

Thanks a lot for giving the example. I didn't quite get what happens in preFixup phase ... if you have some referrences it would be
of great help. Thank you.

It compiles and runs, but I didn't test it (it may require more dependencies for all functionality to work). Just meant as an example to show how these hooks work.

I added gstreamer and gst-plugins-bad in order for video playback to work.

@danieldk
Copy link
Contributor

Thanks a lot for giving the example. I didn't quite get what happens in preFixup phase ... if you have some referrences it would be of great help. Thank you.

It creates a library path for the given derivations. gappsWrapperArgs allows you to pass extra arguments to the wrapper. The nixpkgs manual has documentation on this:

https://nixos.org/nixpkgs/manual/#ssec-gnome-hooks

(Last part of the section.)

I added gstreamer and gst-plugins-bad in order for video playback to work.

Ack.

danieldk
danieldk approved these changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. But I do not know much about GNOME or .NET. But perhaps @jtojnar or @jonringer have some time to look at it.

jtojnar
jtojnar reviewed Aug 19, 2020
View reviewed changes
pkgs/applications/video/gnome-subtitles/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/gnome-subtitles/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/gnome-subtitles/default.nix Show resolved Hide resolved
pkgs/applications/video/gnome-subtitles/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/gnome-subtitles/default.nix Outdated
''
gappsWrapperArgs+=(
--prefix MONO_PATH : "${gtk-sharp-3_0}/lib/mono/gtk-sharp-3.0"
--prefix LD_LIBRARY_PATH : "${libPath}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not fond of setting LD_LIBRARY_PATH since it carries over to child processes, which can cause problems but I have not idea if there is a better way for Mono.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at https://gitlab.gnome.org/GNOME/gnome-subtitles/-/commit/bae6f6d853c70ede461440ce8002913ea1472089#18517c06227b99052bf39e2200684fc54a4f8b8f_3_3, would it be possible to hardcode the library here instead of using LD_LIBRARY_PATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did just that with a patch. It builds fine but the gnome-subtitles executable complains about not finding libglib-2.0.so.0 and I tried fixing that by adding:
<dllmap dll="libglib-2.0-0" target="libglib-2.0.so.0" />
But it does not work.

The patch I applied:

diff --git a/src/GnomeSubtitles/Execution/gnome-subtitles.exe.config b/src/GnomeSubtitles/Execution/gnome-subtitles.exe.config
index fa663ae..c596624 100644
--- a/src/GnomeSubtitles/Execution/gnome-subtitles.exe.config
+++ b/src/GnomeSubtitles/Execution/gnome-subtitles.exe.config
@@ -1,5 +1,7 @@
 <configuration>
 	<dllmap dll="libgtk" target="libgtk-3.so.0" />
-	<dllmap dll="libenchant" target="libenchant.so.1" />
+	<dllmap dll="libenchant" target="libenchant-2.so.2" />
 	<dllmap dll="libgtkspell" target="libgtkspell3-3.so.0" />
+	<dllmap dll="libdl" target="libdl.so" />
+	<dllmap dll="libglib-2.0-0" target="libglib-2.0.so.0" />
 </configuration>

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe try absolute paths in target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe try absolute paths in target?

I went down this path it seems to be a dead end.


libglib = gnome3.glib.out;

  patches = [
    (substituteAll {
      src = ./libraries.patch;
      inherit libglib;
    })
  ];


diff --git a/src/GnomeSubtitles/Execution/gnome-subtitles.exe.config b/src/GnomeSubtitles/Execution/gnome-subtitles.exe.config
index fa663ae..c596624 100644
--- a/src/GnomeSubtitles/Execution/gnome-subtitles.exe.config
+++ b/src/GnomeSubtitles/Execution/gnome-subtitles.exe.config
@@ -1,5 +1,7 @@
 <configuration>
 	<dllmap dll="libgtk" target="libgtk-3.so.0" />
-	<dllmap dll="libenchant" target="libenchant.so.1" />
+	<dllmap dll="libenchant" target="libenchant-2.so.2" />
 	<dllmap dll="libgtkspell" target="libgtkspell3-3.so.0" />
+	<dllmap dll="libdl" target="libdl.so" />
+	<dllmap dll="libglib-2.0-0" target="@libglib@/lib/libglib-2.0.so.0" os="linux" />
 </configuration>

When launching the binary:

Unhandled Exception:
System.DllNotFoundException: libglib-2.0.so.0
  at (wrapper managed-to-native) GLib.Log.g_log_set_default_handler(GLib.Log/LogFuncNative,intptr)
  at GLib.Log.SetDefaultHandler (GLib.LogFunc log_func) [0x0002b] in <3eb93253711f4c08b41956dc83a81a30>:0 
  at GnomeSubtitles.Execution.Executable.Main (System.String[] args) [0x0003a] in <f33d384ba6534837888bffb49690ae3f>:0 
[ERROR] FATAL UNHANDLED EXCEPTION: System.DllNotFoundException: libglib-2.0.so.0
  at (wrapper managed-to-native) GLib.Log.g_log_set_default_handler(GLib.Log/LogFuncNative,intptr)
  at GLib.Log.SetDefaultHandler (GLib.LogFunc log_func) [0x0002b] in <3eb93253711f4c08b41956dc83a81a30>:0 
  at GnomeSubtitles.Execution.Executable.Main (System.String[] args) [0x0003a] in <f33d384ba6534837888bffb49690ae3f>:0

Copy link
Contributor Author

@dasj19 dasj19 Dec 22, 2021 •
edited

Choose a reason for hiding this comment

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

Still not found a way to avoid using LD_LIBRARY_PATH. But the present code works. And the update from 1.6 to 1.7.2 made the packaging a little easier (not having to patch files).

jonringer
jonringer reviewed Aug 20, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

otherwise LGTM with inclusion of @jtojnar 's recommendations

pkgs/applications/video/gnome-subtitles/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

@ofborg eval

jonringer
jonringer reviewed Mar 22, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

pkgconfig is not longer available with allowAliases = false;

pkgs/applications/video/gnome-subtitles/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/gnome-subtitles/default.nix Outdated Show resolved Hide resolved
@locallycompact
Copy link
Contributor

Hi what's the state of this? Can I pick it up?

@SuperSandro2000
Copy link
Member

Hi what's the state of this? Can I pick it up?

Sure, if you want to.

@dasj19
Copy link
Contributor Author

dasj19 commented Apr 30, 2021

I just did small changes to the code. However I don't know how to address this one: #95098 (comment)

@locallycompact you're welcome to take a look... I would like to give a hand if I can, and submit this PR... I can add you as maintainer as well if you want

@stale
Copy link

stale bot commented Oct 30, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 30, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 21, 2021
@dasj19 dasj19 changed the title gnome-subtitles: init at 1.6 gnome-subtitles: init at 1.7.2 Dec 21, 2021
@Artturin Artturin marked this pull request as draft May 1, 2022 23:09
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 30, 2022
@AndersonTorres
Copy link
Member

Closing as dead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@jonringer jonringer jonringer left review comments

@danieldk danieldk danieldk approved these changes

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer 12.approvals: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@dasj19 @danieldk @SuperSandro2000 @locallycompact @AndersonTorres @jtojnar @jonringer @bobby285271 @Artturin