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

linphone: enable gtk ui #46523

Merged
merged 2 commits into from Sep 11, 2018
Merged

linphone: enable gtk ui #46523

merged 2 commits into from Sep 11, 2018

Conversation

stesie
Copy link
Member

@stesie stesie commented Sep 11, 2018 •
edited

Motivation for this change

Linphone voip client currently depends on Gtk, yet the Gtk-based UI is not currently available. This passes ENABLE_GTK_UI=ON to actually enable it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

timokau
timokau previously requested changes Sep 11, 2018
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Does this increase the closure size? If it has any negative consequences it should probably be made an option instead, though it may be enabled by default.

pkgs/applications/networking/instant-messengers/linphone/default.nix Outdated
cmakeFlags = "-DENABLE_GTK_UI=ON";

postPatch = ''
touch coreapi/liblinphone_gitversion.h
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary? What does it do?

Copy link
Member Author

Choose a reason for hiding this comment

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

The source file gtk/main.c depends on this file. Linphone actually has two build systems, one based on cmake, the other one based on autoconf/automake. The coreapi/Makefile.am file actually provides the liblinphone_gitversion.h file (by just touching it), however cmake doesn't. Since otherwise the build would now fail I just touch the file (like coreapi/Makefile.am would)

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a bug, can you please open an upstream ticket for that and link it in a comment?

pkgs/applications/networking/instant-messengers/linphone/default.nix Outdated
@@ -18,6 +18,12 @@ stdenv.mkDerivation rec {
sha256 = "0az2ywrpx11sqfb4s4r2v726avcjf4k15bvrqj7xvhz7hdndmh0j";
};

cmakeFlags = "-DENABLE_GTK_UI=ON";
Copy link
Member

Choose a reason for hiding this comment

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

Its better do use lists for *Flags (except some very specific cases)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@globin
Copy link
Member

globin commented Sep 11, 2018

@timokau this is irrelevant for an application like this, this is already quite large and I'd expect something like this to have the graphical UI enabled, as seemingly was the intention anyway as GTK had been passed in to the buildInputs already.

@stesie
Copy link
Member Author

stesie commented Sep 11, 2018

This change indeed increases the closure size (by roughly 3MB):

$ nix path-info -S /nix/store/3cdpaigzk2dlngmd2mjk13fl0nds4ij2-linphone-3.12.0
/nix/store/3cdpaigzk2dlngmd2mjk13fl0nds4ij2-linphone-3.12.0	  829181032
$ nix path-info -S /nix/store/b92zfm3qqxjhqcqm298j92zjg7qq7jyp-linphone-3.12.0
/nix/store/b92zfm3qqxjhqcqm298j92zjg7qq7jyp-linphone-3.12.0	  832354528

... I've just added a withGui option defaulting to true, that allows to disable it

@timokau
Copy link
Member

timokau commented Sep 11, 2018

Thanks! While 3MB really aren't much here, I think making it clear that it is optional is still better style. With it being enabled by default, we have the best of both worlds.

@globin globin merged commit b6bfbaf into NixOS:master Sep 11, 2018
@timokau
Copy link
Member

timokau commented Sep 11, 2018

It wasn't entirely obsolete:

That sounds like a bug, can you please open an upstream ticket for that and link it in a comment?

We shouldn't just quietly patch around bugs, they at least need to be reported upstream.

@stesie
Copy link
Member Author

stesie commented Sep 11, 2018

@timokau well this has been "fixed" in upstream already, ... the file failing to compile doesn't exist any longer. Actually upstream is porting the user interface to Qt currently.

@stesie stesie deleted the fix-linphone branch September 11, 2018 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timokau timokau timokau left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stesie @globin @timokau @GrahamcOfBorg