Skip to content
This repository has been archived by the owner on Nov 11, 2018. It is now read-only.

Cmake port #20

Merged
merged 6 commits into from
May 13, 2013
Merged

Cmake port #20

merged 6 commits into from
May 13, 2013

Conversation

tom95
Copy link
Contributor

@tom95 tom95 commented May 12, 2013

It got to be a bit more complex than I originally planned.
The terminal now supports (and requires) installing (make install).
The settings desktop file has been removed. Instead, there is a gsettings/dconf replacement, to make per user config easier. The keybindings file would either have to be ported to gsettings as well or be created by the app when a user runs it, or you just leave them the same for all users.
I also moved the file around to make things tidier, we have a cmake folder which hosts the vala related cmake modules, a src folder, which has all the vala files and a vapi folder which has the keybinder-3.0 vapi file. Downloading it manually will no longer be required, except for the library itself of course. (The problem previously was probably that you didn't rename your vapi to keybinder-3.0, so there were some troubles resolving the correct files).
In case you're wondering, the Config.vala.cmake file will be filled by cmake with the requested variables and then put as temporary vala file in the build dir for compilation.

For compilation, just do
mkdir build
cd build
cmake .. # you can add -DCMAKE_INSTALL_PREFIX=/usr, otherwise it will go to /usr/local/share/finalterm

Feel free to change or revert anything you don't like (or reject the entire thing), I'm open for questions or suggestions :)

tom95 added 3 commits May 11, 2013 20:47
Data is now installed globally
Settings have been moved to gsettings/dconf
@p-e-w
Copy link
Owner

p-e-w commented May 12, 2013

o_o This is awesome!

You'll forgive me if I'll need some time to digest all of it before actually merging, which from what I have seen so far I fully intend to do. I'll be nitpicking around the code a bit about style and minor stuff; ideally, I'll want to merge your pull request unchanged.

A few points:

  • The reason why I didn't have make install yet is because I like the fast dev iterations of change -> compile -> launch. Will that still be possible somehow?
  • I'm not sure I like the move to GSettings for one of the config files separately. I have been really impressed by the way Sublime Text 2 handles configurations, everything is a text file (though JSON in that case), everything is in one folder, can be shared, copied, deleted... Also, there is no dependency on GSettings, which might matter for portability reasons.
  • Which platforms have you already compiled this on? I'll try on Fedora and Quantal ASAP, over the next days probably some others as well once I have more time.
  • Any ideas why it is necessary to manually copy the keybinder shared libraries after compilation? I would really like to get rid of that requirement.

I greatly value your contribution. Tonight, I hope to start a development blog for Final Term to coordinate the combined efforts where I will outline a plan towards an initial, "somewhat-stable" release. I will mention your efforts there.

P.S.: The pull request currently doesn't apply to master. I've pushed a few fixes this morning.

@tom95
Copy link
Contributor Author

tom95 commented May 12, 2013

  1. Yep, you can still do that. It will be even faster now, because it won't recompile everything every time now. Only exception is when you change something that is installed globally, that'd apply to the settings scheme or the startup script for example.
  2. For portability, if I'm not entirely mistaken, if on a system is Gtk+-3.0, there'll also be gsettings. GSettings is provided by GLib. With this kind of desktop files per user config may place a challenge, but maybe you can find a way to work around that.
  3. I only compiled it on Raring so far. Unless libkeybinder-3.0 is not available on Quantal or you're using some very new clutter methods, it should work fine there as well.
  4. Sorry, no idea. I'm Ubuntu only and here it works fine :)

Glad you like it so far!

For your P.S., it's actually the first time that I use git, so if I'm required to do something, it'd be great if you could paste the appropriate commands ;)

@p-e-w
Copy link
Owner

p-e-w commented May 12, 2013

You could try to pull again from master (git pull origin master in my repo), but I doubt Git will be able to auto merge since you have moved just about everything. The task is basically to integrate the changes from my branch with yours, which should not be too difficult even manually. We'll see once I'm ready to merge.

@blasphemy
Copy link

Hi, this patch allows finalterm to build for me on arch linux.

Configure works fine:

daniel@rarity ~/work/finalterm (git)-[ef2479d...] % cmake .      
-- The C compiler identification is GNU 4.8.0
-- The CXX compiler identification is GNU 4.8.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Found PkgConfig: /usr/bin/pkg-config (found version "0.28") 
-- checking for modules 'clutter-gtk-1.0;mx-1.0;gee-0.8;keybinder-3.0'
--   found clutter-gtk-1.0, version 1.4.4
--   found mx-1.0, version 1.4.6
--   found gee-0.8, version 0.10.1
--   found keybinder-3.0, version 0.3.0
-- Found Vala: /usr/bin/valac  
-- checking for a minimum Vala version of 0.16.0
--   found Vala, version 0.20.1
-- GSettings schemas will be installed locally.
-- GSettings shemas will be compiled.
-- GSettings schemas will be installed into /usr/local/share/glib-2.0/schemas/
-- Configuring done
-- Generating done
-- Build files have been written to: /home/daniel/work/finalterm

And it builds fine (ignoring numerous warnings that are extremely common with vala projects), but I get a couple of linker errors.

Linking C executable finalterm
/usr/bin/ld: warning: libcogl-pango.so.0, needed by /usr/lib/gcc/x86_64-unknown-linux-gnu/4.8.0/../../../../lib/libmx-1.0.so, not found (try using -rpath or -rpath-link)
/usr/bin/ld: warning: libcogl.so.9, needed by /usr/lib/gcc/x86_64-unknown-linux-gnu/4.8.0/../../../../lib/libmx-1.0.so, not found (try using -rpath or -rpath-link)

Edit: seems both libraries are available on my system, they just don't have the soname it's complaining about

daniel@rarity ~/work/finalterm (git)-[ef2479d...] % whereis libcogl-pango
libcogl-pango: /usr/lib/libcogl-pango.so
daniel@rarity ~/work/finalterm (git)-[ef2479d...] % whereis libcogl
libcogl: /usr/lib/libcogl.so

@tom95
Copy link
Contributor Author

tom95 commented May 12, 2013

Would you mind me fixing more stuff in this branch, unrelated to cmake? For example I found the cause of the endless "queueing a relayout is not recommended" warnings, and I'd also like to get rid of all the warnings at compile time, because this mass of output sometimes makes it hard to spot the error amongst the warnings, which makes the compilation fail. If not I'll just wait till the branch's merged and make a new pull request for those :)

@p-e-w
Copy link
Owner

p-e-w commented May 12, 2013

@TomB95: The queuing bug? REALLY? That one's been bugging me for ages, but I never quite felt like going too deep trying to find the cause. Wonderful!

As for getting rid of all compile time warnings, good luck. While most of the Vala ones are simply unhandled errors, Vala has numerous bugs that let problems slip through to GCC without it noticing (both warnings and errors, many of them documented in Final Term's source code, which I imagine might become a treasure trove of test cases for the Vala makers). I doubt it is possible to eliminate all warnings.

As for all this magic going into this branch... I somehow would prefer if those were separate pull requests made afterwards – which now makes merging the CMake stuff my number one priority, and I'll try hard to get that done tomorrow.

Keep going :)

@p-e-w
Copy link
Owner

p-e-w commented May 12, 2013

@TomB95: I looked through the entire pull request and feel confident to merge now, problem is, it doesn't apply to the current master branch. If you can fix that I'll merge immediately, otherwise I'll try manually tomorrow.

BTW I noticed tom95@89154e2, if that is necessary for the pull request to work properly feel free to add it.

@p-e-w
Copy link
Owner

p-e-w commented May 12, 2013

Now here's the ugly part: On Fedora, trying to build with cmake, I am getting

-- checking for modules 'clutter-gtk-1.0;mx-1.0;gee-0.8;keybinder-3.0'
--   package 'keybinder-3.0' not found
CMake Error at /usr/share/cmake/Modules/FindPkgConfig.cmake:279 (message):
  A required package was not found
Call Stack (most recent call first):
  /usr/share/cmake/Modules/FindPkgConfig.cmake:333 (_pkg_check_modules_internal)
  CMakeLists.txt:21 (pkg_check_modules)

If I change "keybinder-3.0" to "keybinder" as in the Makefile I get the same error:

-- checking for modules 'clutter-gtk-1.0;mx-1.0;gee-0.8;keybinder'
--   package 'keybinder' not found
CMake Error at /usr/share/cmake/Modules/FindPkgConfig.cmake:279 (message):
  A required package was not found
Call Stack (most recent call first):
  /usr/share/cmake/Modules/FindPkgConfig.cmake:333 (_pkg_check_modules_internal)
  CMakeLists.txt:21 (pkg_check_modules)

Although keybinder can be found when I run the makefile. Me knowing nothing about CMake, do you have an idea what is going on?

@tom95
Copy link
Contributor Author

tom95 commented May 13, 2013

I guess you haven't made pkg-config aware of the package. You can check that by running "pkg-config --list-all | grep keybinder". It should show keybinder-3.0 then.
The file responsible for that is found at /usr/lib/x86_64-linux-gnu/pkgconfig/keybinder-3.0.pc for me, not sure where that'd be on fedora. Maybe you got to copy that one manually as well :/

For the changes, you could either wait for me to come back home in around 10 hours from now or change them yourself when merging if you don't feel like waiting, I agree with all your remarks :)

@p-e-w
Copy link
Owner

p-e-w commented May 13, 2013

I'll be going out now myself and be back in about 10 hours too. I guess we live in the same part of the world :)

Will be trying that out then.

@varemenos
Copy link

I'm not sure I like the move to GSettings for one of the config files separately. I have been really impressed by the way Sublime Text 2 handles configurations, everything is a text file (though JSON in that case), everything is in one folder, can be shared, copied, deleted... Also, there is no dependency on GSettings, which might matter for portability reasons.

With the current setup, will it work on other desktop enviroments other than Gnome? for example, KDE. If not, then I'd go for the .json file for the settings.

Maybe, there should be another bug filed where there can be further discussion about this.

@p-e-w
Copy link
Owner

p-e-w commented May 13, 2013

@varemenos: See #20 (comment)

@p-e-w
Copy link
Owner

p-e-w commented May 13, 2013

@TomB95:

Found it:

PKG_CONFIG_PATH=/usr/local/lib/pkgconfig/
export PKG_CONFIG_PATH
cmake ..
make
sudo make install

And WOW, does it work! And how many issues it solves. Turns out that the entire library copying hack can be done away with if the pkg-config search path is pointed to the (unusual) location where the libkeybinder make install puts it.

Let's get this merged!

@p-e-w
Copy link
Owner

p-e-w commented May 13, 2013

@TomB95: I'm doing the merge right now, no need for you to fiddle. Will let you know when it's done.

@p-e-w p-e-w mentioned this pull request May 13, 2013
@tom95
Copy link
Contributor Author

tom95 commented May 13, 2013

@varemenos it will run the same way Gedit or basically any other Gtk app would run on Kde.

@p-e-w p-e-w merged commit ef2479d into p-e-w:master May 13, 2013
@p-e-w
Copy link
Owner

p-e-w commented May 13, 2013

Done!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants