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.

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

Goodbye Autotools, hello CMake and minizip #147

Merged
merged 2 commits into from
Jun 21, 2015
Merged

Goodbye Autotools, hello CMake and minizip #147

merged 2 commits into from
Jun 21, 2015

Conversation

vorot93
Copy link
Contributor

@vorot93 vorot93 commented Jun 18, 2015

Now this is a huge patch which overhauls the whole XQF build process.

I have already explained at length why the current build system (GNU Autotools) needs to go. This patchset removes autotools support completely and adds CMake.

Key highlights:

  • New cross-platform CMake build system. It is available for all major Unixen as well as Windows.
  • Single build recipe instead of over 30 autotools config files
  • Out-of-tree compilation. No more binary garbage in source directories!
  • .gitignore slimmed to a single build entry. See above.
  • minizip is now a dependency. XQF no longer ships its own copy of unzip.h and unzip.c
  • gettext is now a hard dependency. I hope it's obvious why: it's 2015, not 1990.
  • Killed bzip2 support. Pretty much explains itself, gz and xz are the two standards today, bz2 times are long gone.

How to build:

mkdir build
cd build
cmake (your flags here) .. && make && make install

Flags (prefixed with -D)

  • CMAKE_INSTALL_PREFIX=path - root directory of installation, default: /usr/local
  • WITH_QSTAT=path - qstat path, default: /usr/bin/qstat
  • USE_GTK3=ON/OFF - build against gtk3 toolkit, default: OFF
  • DEPRECATED_DISABLE=ON/OFF - disable deprecated features in GLib/GObject/GDK/GTK stack, default: OFF
  • USE_GEOIP=ON/OFF - use geoip or not, default: OFF
  • USE_GZIP=ON/OFF - include gzip support, default: OFF
  • RCON_STANDALONE=ON/OFF - build separate rcon, default: OFF

This is a very extensive overhaul, may require some testing. Works for me but some bugs might remain.

Fixes #138 #141

@zturtleman
Copy link
Contributor

Travis CI runs Ubuntu 14.04 which does not have a minizip package.

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 18, 2015

@zturtleman If only. Ubuntu Trusty (14.04) does have /usr/include/minizip/unzip.h in libkml-dev package. Ubuntu Precise (12.04), however, does not.

Travis runs Precise.

@zturtleman
Copy link
Contributor

Oh, I forgot it wasn't the newest Ubuntu LTS.

missing
po/stamp-it
src/gamesxml2c
.*.swp
Copy link
Member

Choose a reason for hiding this comment

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

.*.swp files are not build files, they are temporary files created by vim, the editor. gitignoring them prevents users to commit temporary files while they edit some files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://news.ycombinator.com/item?id=1688068

There is no place in tree for such garbage to start with. Gitignoring it makes your sources look like a hoarder's trash can.

Copy link
Member

Choose a reason for hiding this comment

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

it’s a valid argument

@illwieckz
Copy link
Member

Woaw, it’s a very good job ! 👍

I like a lot that:

Showing with 263 additions and 3,385 deletions.

😍

@jmallach, @lnussel, I know these patches will change the way you build packages but I hope this job will facilitate a lot everyone’s build procedures in long term.

@Slashbunny, when this PR will be merged, you will need to update your xqf-git build script too! That's why I poke you now. 😉

So, @jmallach, @lnussel, and @Slashbunny, if you have some suggestions to help distro packaging, feel free to write down them. This work is made to help everyone, including you ! 😏

@@ -5,12 +5,12 @@ compiler:
- gcc

before_install:
- wget -q -O- https://launchpad.net/ubuntu/+archive/primary/+files/libminizip-dev_1.1-5_amd64.deb | sudo dpkg -i -
Copy link
Member

Choose a reason for hiding this comment

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

see https://travis-ci.org/XQF/xqf/jobs/67411920
Try something like that:

wget -q -O/tmp/libminizip-dev_1.1-5_amd64.deb https://launchpad.net/ubuntu/+archive/primary/+files/libminizip-dev_1.1-5_amd64.deb
sudo dpkg -i /tmp/libminizip-dev_1.1-5_amd64.deb

But a more elegant way could be to do apt repository pinning, if travis-ci allows it.

@illwieckz
Copy link
Member

about https://travis-ci.org/XQF/xqf/jobs/67418533

it seems the pinning way is the only way

@illwieckz
Copy link
Member

@skybon: try something like that (not tested):

test "$(lsb_release -sr | cut -c1-2)" -lt '15' && (echo 'deb http://archive.ubuntu.com/ubuntu vivid main' > '/etc/apt/sources.list.d/ubuntu-vivid-main.list'; printf 'Package: *\nPin: release n=vivid\nPin-Priority: -100\n' > '/etc/apt/preferences.d/vivid-pinning'; apt-get update; apt-get install -y -t vivid cmake)

If there is no mistake in this code, it means “if distro is older than 15.04, add the vivid repository, blacklist the whole vivid repository, then explicitly install cmake and its dependencies (and only its dependencies) from the vivid repository”.

@illwieckz
Copy link
Member

Hey, well, Why I’m testing/installing cmake to install minizip ? I had an hard day today, my mind is blowing.

This is better:

test "$(lsb_release -sr | cut -c1-2)" -lt '15' && (echo 'deb http://archive.ubuntu.com/ubuntu vivid main' > '/etc/apt/sources.list.d/ubuntu-vivid-main.list'; printf 'Package: *\nPin: release n=vivid\nPin-Priority: -100\n' > '/etc/apt/preferences.d/vivid-pinning'; apt-get update; apt-get install -y -t vivid libminizip-dev)

If there is no mistake in this code, it means “if distro is older than 15.04, add the vivid repository, blacklist the whole vivid repository, then explicitly install libminizip-dev and its dependencies (and only its dependencies) from the vivid repository”.

@illwieckz
Copy link
Member

This attempt does not use \n chars:

test "$(lsb_release -sr | cut -c1-2)" -lt '15' && (echo "deb http://archive.ubuntu.com/ubuntu vivid main" > "/etc/apt/sources.list.d/ubuntu-vivid-main.list"; (echo 'Package: *'; echo 'Pin: release n=vivid'; echo 'Pin-Priority: -100') > /etc/apt/preferences.d/vivid-pinning; apt-get update; apt-get install -t vivid cmake)

@jmallach
Copy link
Contributor

I am totally unconvinced we need to switch build systems. I see no real feature in what Artem outlines, but I see it makes us diverge from what the gtk world does.

Not really convinced. :(

On 18 juny de 2015 17:41:02 CEST, Artem Vorotnikov [email protected] wrote:

Now this is a huge patch which overhauls the whole XQF build process.

I have already explained at length why the current build system (GNU
Autotools) needs to go. This patchset removes autotools support
completely and adds CMake.

Key highlights:

  • New cross-platform CMake build system. It is available for all major
    Unixen as well as Windows.
  • Out-of-tree compilation. No more binary garbage in source
    directories!
  • .gitignore slimmed to build directory. See above.
  • minizip is now a dependency. XQF no longer ships its own copy of
    unzip.h and unzip.c
  • gettext is now a hard dependency. I hope it's obvious why: it's
    2015, not 1990.
  • Killed bzip2 support. Pretty much explains itself, gz and xz are the
    two standards today, bz2 times are long gone.

How to build:

mkdir build
cd build
cmake (your flags here) .. && make && make install

Flags (prefixed with -D)

  • WITH_QSTAT=path - qstat path, default: /usr/bin/qstat
  • USE_GTK3=ON/OFF - build against gtk3 toolkit, default: OFF
  • DEPRECATED_DISABLE=ON/OFF - disable deprecated features in
    GLib/GObject/GDK/GTK stack/ default: OFF
  • USE_GEOIP=ON/OFF - use geoip or not, default: OFF
  • USE_GZIP=ON/OFF - include gzip support, default: OFF
  • RCON_STANDALONE=ON/OFF - build separate rcon, default: OFF

This is a very extensive overhaul, may require some testing. Works for
me but some bugs might remain.
You can view, comment on, or merge this pull request online at:

#147

-- Commit Summary --

  • Goodbye Autotools, hello CMake and minizip

-- File Changes --

M .gitignore (41)
A CMakeLists.txt (237)
D Makefile.am (30)
D autogen.sh (168)
D clean.sh (64)
D configure.ac (236)
D docs/.gitignore (2)
D docs/Makefile.am (1)
D pixmaps/.gitignore (2)
D pixmaps/128x128/Makefile.am (10)
D pixmaps/22x22/Makefile.am (10)
D pixmaps/32x32/Makefile.am (10)
D pixmaps/48x48/Makefile.am (10)
D pixmaps/Makefile.am (6)
D pixmaps/flags/.gitignore (3)
D pixmaps/flags/Makefile.am (4)
D pixmaps/scalable/Makefile.am (10)
D po/.gitignore (14)
D po/ChangeLog (75)
D po/LINGUAS (8)
D po/Makefile.in.in (222)
D po/Makevars (41)
D po/POTFILES.in (39)
D src/.gitignore (12)
D src/Makefile.am (206)
M src/addmaster.c (2)
M src/addserver.c (2)
M src/country-filter.c (1)
M src/dialogs.c (2)
M src/filter.c (2)
M src/flt-player.c (2)
M src/game.c (2)
M src/launch.c (3)
M src/loadpixmap.c (2)
M src/menus.c (3)
M src/pixmaps.c (3)
M src/pref.c (2)
M src/psearch.c (3)
M src/q3maps.c (4)
M src/rcon.c (10)
M src/redial.c (2)
M src/scripts.c (4)
M src/source.c (2)
M src/srv-list.c (2)
M src/srv-prop.c (2)
M src/stat.c (11)
M src/stat.h (6)
M src/statistics.c (2)
M src/test_utils.c (1)
D src/tga/.gitignore (3)
D src/tga/Makefile.am (21)
M src/tga/memtopixmap.c (2)
M src/utils.c (2)
M src/utmaps.c (2)
D src/xpm/.gitignore (2)
D src/xpm/Makefile.am (16)
M src/xqf-ui.c (2)
M src/xqf.c (9)
M src/xqf.h (2)
D src/zip/.gitignore (3)
D src/zip/Makefile.am (2)
D src/zip/README (5)
D src/zip/ioapi.c (177)
D src/zip/ioapi.h (79)
D src/zip/unzip.c (1436)
D src/zip/unzip.h (323)
M src/zipped.c (12)
M xqf.desktop.in (8)

-- Patch Links --

https://github.com/XQF/xqf/pull/147.patch
https://github.com/XQF/xqf/pull/147.diff


Reply to this email directly or view it on GitHub:
#147

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 19, 2015

@jmallach Can you explain the contents of every one of 30 Autotools files, part by part? I certainly can for the CMake listfile.

The transparency and maintainability is the first and foremost feature of any build system. What if we happen to develop a feature that requires a new dependency? How are you going to modify build system? It'll take us exactly 5 minutes to enable it in CMake. Not so fast with Autohell.

Moreover, current build toolchain leaves your source tree garbled with intermediary and binary files, forcing you to use clean.sh if pristine sources are of value to you.

And what is 'GTK world', GNOME? GNOME is a GNU project and (afaik) is by policy required to use Autohell. GNU is a bit stuck in 80s where int might not be int and sys/ioctl.h is not a must have.

GTK is a toolkit, CMake detects it and works without any hassle with 5 lines of code. That's the only thing that matters.

@illwieckz
Copy link
Member

@jmallach said:

I see it makes us diverge from what the gtk world does.

Valid argument ✅

@skybon said:

Can you explain the contents of every one of 30 Autotools files, part by part? I certainly can for the CMake listfile.

Valid argument ✅

@skybon said:

The transparency and maintainability is the first and foremost feature of any build system.

Valid argument ✅

@skybon said:

And what is 'GTK world', GNOME? GNOME is a GNU project and (afaik) is by policy required to use Autohell. GNU is a bit stuck in 80s where int might not be int and sys/ioctl.h is not a must have.

Invalid argument ❌

@jmallach said:

I see no real feature in what Artem outlines

Invalid argument ❌

I personally have no preference for a build system or another, but personally I have a strong preference for solutions that are simpler than others. Whatever the solution is, if it's simpler, it's better.

The current build system is very problematic :

  • Building inside versioned source tree is not good, I don't want to rely on some hacks like the .gitignore file, it's the best way to commit accidentaly some files when you add some features. @skybon is fixing this issue. OK, I don't know if he can fix this issue with autotools too, but he is fixing this issue with cmake.
  • The make gamelist is very problematic. In fact, the idea that we must compile the code to be able to commit the code is very problematic. @skybon is fixing this issue. OK, perhaps he can fix this issue with autotools too, but he is fixing this issue with cmake.
  • When building code outside the versioned source tree, we must copy back the generated games.c to the versioned source tree after compilation. It’s the best way to mistakenly commit incomplete changes. @skybon is fixing this issue. OK, perhaps he can fix this issue with autotools too, but he is fixing this issue with cmake.
  • Some code can be externalized (like minizip) to reduce the surface to maintain. @skybon is fixing this issue. OK, perhaps he can fix this issue with autotools too, but he is fixing this issue with cmake.
  • The build procedure is very verbose and cryptic. @skybon is fixing this issue. OK, perhaps he can't fix this issue with autotools, but he is fixing this issue with cmake.

For your information, this is how I build xqf with the old autotools implementation:

sourcedir="${HOME}/dev/xqf"
builddir="${HOME}/build/xqf"

mkdir -p "${builddir}"
rsync -a -v --delete-after "${sourcedir}/." "${builddir}/."
cd "${builddir}"

./autogen.sh
./configure --with-qstat="/usr/bin/quakestat" --prefix="/usr"
cd "${builddir}/src"
make gamelist
cp -a "${builddir}/src/games.c" "${sourcedir}/src/games.c"
cd "${builddir}"
make

fakeroot checkinstall --default --fstrans=yes --install=no --pkgname=xqf --pkgversion="1.0.6.2~git" make install

This is how I build xqf with the current @skybon's cmake implementation:

sourcedir="${HOME}/dev/xqf"
builddir="${HOME}/build/xqf"

mkdir -p "${builddir}"
cd "${builddir}"

cmake -DWITH_QSTAT="/usr/bin/quakestat" -DCMAKE_INSTALL_PREFIX="/usr" "${sourcedir}"
make 

fakeroot checkinstall --default --fstrans=yes --install=no --pkgname=xqf --pkgversion="1.0.6.2~git" make install

I have no preference for a particular build system, but I have a preference for which simplifies the maintenance work, and in this regard, whatever the solution he uses, skybon is doing the job and it seems his is doing it well.

That said @jmallach, I do not want to offend you. I have great esteem for your work and all that time you spent to maintain xqf all these years. I am obliged to gratitude towards you, and @skybon is obliged to gratitude towards you for this reason. We could not do this job today if you had not maintained xqf all these years, so your opinion is very valuable.

When @skybon started to tell me about his initiative to convert the build system to cmake, I told him he should convince you first, and I told him that he must make your job easier.

So, I think the current work @skybon is doing is very valuable and very good, but if you need something more, please ask it to him. This current effort is not done to annoy you but to help you. If there is some problems to fix in this implementation, please ask @skybon to fix them, but please, let's use this beautiful work.

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 19, 2015

On Travis failures

@illwieckz
Copy link
Member

@skybon, do not give up too quickly. 😛

Use that in the .travis.yaml file:

before_install:
  - sudo apt-get update -qq
  - sudo apt-get install -q -y qstat intltool
  - echo 'install libminizip-dev from ubuntu vivid if current release is older than vivid:'
  - echo 'c2V0IC14CgojIGlmIHJlbGVhc2Ugb2xkZXIgdGhhbiB2aXZpZCAoMTUuMDQpCmlmIFsgIiQobHNiX3JlbGVhc2UgLXNyIHwgY3V0IC1jMS0yKSIgLWx0ICcxNScgXQp0aGVuCgoJIyBhZGQgdml2aWQgcmVwb3NpdG9yeSAobGlibWluaXppcC1kZXYgaXMgc2hpcGVkIGluIHVuaXZlcnNlKQoJY2F0ID4gJy9ldGMvYXB0L3NvdXJjZXMubGlzdC5kL3VidW50dS12aXZpZC1tYWluLmxpc3QnIDw8LUVPRgoJZGViIGh0dHA6Ly9hcmNoaXZlLnVidW50dS5jb20vdWJ1bnR1IHZpdmlkIHVuaXZlcnNlCglFT0YKCgkjIHBpbiB2aXZpZCByZXBvc2l0b3J5IHRvIGRpc2FibGUgcGFja2FnZSBpbnN0YWxsYXRpb24gZnJvbSB0aGlzIHJlcG9zaXRvcnkgYnkgZGVmYXVsdAoJY2F0ID4gJy9ldGMvYXB0L3ByZWZlcmVuY2VzLmQvdml2aWQtcGlubmluZycgPDwtRU9GCglQYWNrYWdlOiAqCglQaW46IHJlbGVhc2Ugbj12aXZpZAoJUGluLVByaW9yaXR5OiAtMTAwCglFT0YKCgkjIGFkZCB0aGUgdml2aWQgcmVwb3NpdG9yeSBrZXkKCWFwdC1rZXkgYWR2IC0tcmVjdi1rZXlzIC0ta2V5c2VydmVyIGtleXNlcnZlci51YnVudHUuY29tIDNCNEZFNkFDQzBCMjFGMzIKCgkjIHVwZGF0ZSB0aGUgcmVwb3NpdG9yaWVzCglhcHQtZ2V0IHVwZGF0ZSAtcXEKCgkjIGluc3RhbGwgbGlibWluaXppcC1kZXYgKGFuZCBkZXBlbmRlbmNpZXMpIGZyb20gdml2aWQKCWFwdC1nZXQgaW5zdGFsbCAteSAtcSAtdCB2aXZpZCBsaWJtaW5pemlwLWRldgpmaQo=' | base64 -d | sudo -s
  - ./autogen.sh

The cryptic line is an encoded script that does:

set -x

# if release older than vivid (15.04)
if [ "$(lsb_release -sr | cut -c1-2)" -lt '15' ]
then

    # add vivid repository (libminizip-dev is shiped in universe)
    cat > '/etc/apt/sources.list.d/ubuntu-vivid-main.list' <<-EOF
    deb http://archive.ubuntu.com/ubuntu vivid universe
    EOF

    # pin vivid repository to disable package installation from this repository by default
    cat > '/etc/apt/preferences.d/vivid-pinning' <<-EOF
    Package: *
    Pin: release n=vivid
    Pin-Priority: -100
    EOF

    # add the vivid repository key
    apt-key adv --recv-keys --keyserver keyserver.ubuntu.com 3B4FE6ACC0B21F32

    # update the repositories
    apt-get update -qq

    # install libminizip-dev (and dependencies) from vivid
    apt-get install -y -q -t vivid libminizip-dev
fi

And it seems that Travis CI like it. 😉

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 19, 2015

@illwieckz Props for creativity, it works! 👍

@illwieckz
Copy link
Member

Just a thing, some shells does not accept to append to non existent files (so, the safer is to touch file before >> file. 😉

@illwieckz
Copy link
Member

Just for fun, building xqf source tree with autotools autoupdate the file Makefile.in.in:

------------------------------ po/Makefile.in.in ------------------------------
index 06a8cfe..fcd2c3b 100644
@@ -33,8 +33,7 @@ exec_prefix = @exec_prefix@
 datadir = @datadir@
 datarootdir = @datarootdir@
 libdir = @libdir@
-DATADIRNAME = @DATADIRNAME@
-itlocaledir = $(prefix)/$(DATADIRNAME)/locale
+localedir = @localedir@
 subdir = po
 install_sh = @install_sh@
 # Automake >= 1.8 provides @mkdir_p@.
@@ -80,7 +79,7 @@ INTLTOOL__v_MSGFMT_0 = @echo "  MSGFMT" $@;

 .po.pox:
    $(MAKE) $(GETTEXT_PACKAGE).pot
-   $(MSGMERGE) $< $(GETTEXT_PACKAGE).pot -o $*.pox
+   $(MSGMERGE) $* $(GETTEXT_PACKAGE).pot -o $*.pox

 .po.mo:
    $(INTLTOOL_V_MSGFMT)$(MSGFMT) -o $@ $<
@@ -108,7 +107,7 @@ install-data-no: all
 install-data-yes: all
    linguas="$(USE_LINGUAS)"; \
    for lang in $$linguas; do \
-     dir=$(DESTDIR)$(itlocaledir)/$$lang/LC_MESSAGES; \
+     dir=$(DESTDIR)$(localedir)/$$lang/LC_MESSAGES; \
      $(mkdir_p) $$dir; \
      if test -r $$lang.gmo; then \
        $(INSTALL_DATA) $$lang.gmo $$dir/$(GETTEXT_PACKAGE).mo; \
@@ -142,8 +141,8 @@ install-exec installcheck:
 uninstall:
    linguas="$(USE_LINGUAS)"; \
    for lang in $$linguas; do \
-     rm -f $(DESTDIR)$(itlocaledir)/$$lang/LC_MESSAGES/$(GETTEXT_PACKAGE).mo; \
-     rm -f $(DESTDIR)$(itlocaledir)/$$lang/LC_MESSAGES/$(GETTEXT_PACKAGE).mo.m; \
+     rm -f $(DESTDIR)$(localedir)/$$lang/LC_MESSAGES/$(GETTEXT_PACKAGE).mo; \
+     rm -f $(DESTDIR)$(localedir)/$$lang/LC_MESSAGES/$(GETTEXT_PACKAGE).mo.m; \
    done

 check: all $(GETTEXT_PACKAGE).pot

Since I build out of tree, theses changes are never committed.

This autotools stuff needs to build the project to be able to commit the source, it's weird…
Also, it means autotools can silently add some changes in some of your own unrelated patch and silently pollute your pull requests…

@illwieckz
Copy link
Member

@skybon: it's not a critical issue, but currently, -DUSE_GTK3=ON does not trigger the GTK3 compilation:

[illwieckz@ubuntu:/home/illwieckz/dev/xqf-skybon/build] cmake ± cmake -DUSE_GTK3=ON ..
-- The C compiler identification is GNU 4.9.2
-- The CXX compiler identification is GNU 4.9.2
-- 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") 
-- Found Gettext: /usr/bin/msgmerge (found version "0.19.2") 
-- checking for module 'minizip'
--   found minizip, version 1.2.8
-- checking for module 'libxml-2.0'
--   found libxml-2.0, version 2.9.2
-- checking for module 'gtk+-2.0'
--   found gtk+-2.0, version 2.24.27
-- Configuring done
-- Generating done
-- Build files have been written to: /home/illwieckz/dev/xqf-skybon/build

@illwieckz
Copy link
Member

Hi @skybon, it seems you miss something about GeoIP:

https://travis-ci.org/XQF/xqf/jobs/67608335

[ 12%] Building C object CMakeFiles/xqf.dir/src/country-filter.c.o

/home/travis/build/XQF/xqf/src/country-filter.c:34:19: fatal error: GeoIP.h: No such file or directory

compilation terminated.

make[2]: *** [CMakeFiles/xqf.dir/src/country-filter.c.o] Error 1

make[1]: *** [CMakeFiles/xqf.dir/all] Error 2

make: *** [all] Error 2

The command "make" exited with 2.

Done. Your build exited with 1.

😜

@illwieckz
Copy link
Member

Hi @skybon,

Two things:

  • It seems make install does not set the executable permission bit for /usr/bin/xqf
  • The country flags in statistic window seem missing

Otherwise the rest seems ok to me. 😃

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 21, 2015

  • Fixed
  • Not my bug, flags have been absent from repository for quite some time now.

@illwieckz
Copy link
Member

  • Thanks
  • Too bad 😕

@jmallach it's you who had removed flags.tar.gz, but you were fooled by the very complex build system and .gitignore system.

In autotools' age, flags.tar.gz was extracted by ./autogen.sh, and flags extracted were ignored by git because of pixmaps/flags/.gitignore so after the flags.tar.gz remove, the flags were lost because they were never committed.

We lose the flags because we use autotools so you are building inside versioned source tree.

Also, I made before a comment about a versioned file po/makefile.in.in modified by autotools, in fact, this file must not be present in versioned source tree, it's a temporary file generated at build time by autotools. @jmallach this file was added by your 15_fix_intltool.patch from Debian. Yes I know, it's me who had applied the patch to the xqf source tree, but the Makefile.in.in is shipped by your file, so Debian is shipping this temporary file in its source repository (and the xqf source package) since 2013-06-28.

So, I'm sorry, @jmallach, unless I misunderstood something, you made two mistakes because we are using autotools and because you are building inside versioned source tree. It's not your fault, it's autotools fault, so we need to get rid of autotools.

  • All the flags were lose since 2015-02-10 because we use autotools.
  • One temporary build file is tracked by source repository since 2013-11-02 and Debian is tracking this file in his own source repository since 2013-06-28 because we use autotools.

We've done all these mistakes only because we use autotools and we build inside the versionned source tree, because the autotools stuff is unreadable, and because the .gitignore stuff is fooling us.

So,

  • we need cmake.
  • we need out-of-tree compilation.

@illwieckz illwieckz added this to the XQF 1.0.7 milestone Jun 21, 2015
fi

# extract flag icons
rm pixmaps/flags/*.png
Copy link
Member

Choose a reason for hiding this comment

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

Here the flags were deleted if they are already there (not expected to be in the repository).

@illwieckz
Copy link
Member

So this PR is fixing:

@skybon, can you re-add the flags so this PR will fix #156 too? 😉

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 21, 2015

Perhaps you could merge current toolchain first, this PR is already bloated as it is.

@illwieckz
Copy link
Member

OK. It seems I have to merge this PR. We can't wait more.

With all the problems this PR revealed and all the problems this PR fixes, autotools is not a valid choice anymore.

This was referenced Jun 21, 2015
@illwieckz
Copy link
Member

@skybon, just one last thing:

[illwieckz@ubuntu:/home/illwieckz/dev/xqf-skybon/build] cmake ± cmake -DWITH_QSTAT=/usr/bin/quakestat -DCMAKE_INSTALL_PREFIX=/usr ..
-- The C compiler identification is GNU 4.9.2
-- The CXX compiler identification is GNU 4.9.2
-- 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") 
-- Found Gettext: /usr/bin/msgmerge (found version "0.19.2") 
-- checking for module 'minizip'
--   found minizip, version 1.2.8
-- checking for module 'libxml-2.0'
--   found libxml-2.0, version 2.9.2
-- checking for module 'gtk+-2.0'
--   found gtk+-2.0, version 2.24.27
-- checking for module 'geoip'
--   found geoip, version 1.6.3
-- Configuring done
-- Generating done
-- Build files have been written to: /home/illwieckz/dev/xqf-skybon/build
[illwieckz@ubuntu:/home/illwieckz/dev/xqf-skybon/build] cmake ± make
Scanning dependencies of target gamesxml2c
[  2%] Building C object CMakeFiles/gamesxml2c.dir/src/gamesxml2c.c.o
Linking C executable gamesxml2c
[  2%] Built target gamesxml2c
Scanning dependencies of target translation
................................................. terminé.
.................................................... terminé.
................................................... terminé.
................................................ terminé.
............................................... terminé.
.................................................. terminé.
.............................................. terminé.
.............................................. terminé.
[  2%] Built target translation
Scanning dependencies of target xqf
[  4%] Building C object CMakeFiles/xqf.dir/src/addmaster.c.o
[  7%] Building C object CMakeFiles/xqf.dir/src/addserver.c.o
[  9%] Building C object CMakeFiles/xqf.dir/src/config.c.o
[ 12%] Building C object CMakeFiles/xqf.dir/src/country-filter.c.o
[ 14%] Building C object CMakeFiles/xqf.dir/src/debug.c.o
[ 17%] Building C object CMakeFiles/xqf.dir/src/dialogs.c.o
[ 19%] Building C object CMakeFiles/xqf.dir/src/dns.c.o
[ 21%] Building C object CMakeFiles/xqf.dir/src/filter.c.o
[ 24%] Building C object CMakeFiles/xqf.dir/src/flt-player.c.o
[ 26%] Building C object CMakeFiles/xqf.dir/src/game.c.o
[ 29%] Building C object CMakeFiles/xqf.dir/src/history.c.o
[ 31%] Building C object CMakeFiles/xqf.dir/src/host.c.o
[ 34%] Building C object CMakeFiles/xqf.dir/src/launch.c.o
[ 36%] Building C object CMakeFiles/xqf.dir/src/menus.c.o
[ 39%] Building C object CMakeFiles/xqf.dir/src/pixmaps.c.o
[ 41%] Building C object CMakeFiles/xqf.dir/src/pref.c.o
[ 43%] Building C object CMakeFiles/xqf.dir/src/psearch.c.o
[ 46%] Building C object CMakeFiles/xqf.dir/src/rc.c.o
[ 48%] Building C object CMakeFiles/xqf.dir/src/rcon.c.o
[ 51%] Building C object CMakeFiles/xqf.dir/src/server.c.o
[ 53%] Building C object CMakeFiles/xqf.dir/src/skin.c.o
[ 56%] Building C object CMakeFiles/xqf.dir/src/skin_pcx.c.o
[ 58%] Building C object CMakeFiles/xqf.dir/src/sort.c.o
[ 60%] Building C object CMakeFiles/xqf.dir/src/source.c.o
[ 63%] Building C object CMakeFiles/xqf.dir/src/srv-info.c.o
[ 65%] Building C object CMakeFiles/xqf.dir/src/srv-list.c.o
[ 68%] Building C object CMakeFiles/xqf.dir/src/srv-prop.c.o
[ 70%] Building C object CMakeFiles/xqf.dir/src/stat.c.o
[ 73%] Building C object CMakeFiles/xqf.dir/src/statistics.c.o
[ 75%] Building C object CMakeFiles/xqf.dir/src/utils.c.o
[ 78%] Building C object CMakeFiles/xqf.dir/src/xqf.c.o
[ 80%] Building C object CMakeFiles/xqf.dir/src/xqf-ui.c.o
[ 82%] Building C object CMakeFiles/xqf.dir/src/zipped.c.o
[ 85%] Building C object CMakeFiles/xqf.dir/src/redial.c.o
[ 87%] Building C object CMakeFiles/xqf.dir/src/q3maps.c.o
[ 90%] Building C object CMakeFiles/xqf.dir/src/utmaps.c.o
[ 92%] Building C object CMakeFiles/xqf.dir/src/loadpixmap.c.o
[ 95%] Building C object CMakeFiles/xqf.dir/src/scripts.c.o
[ 97%] Building C object CMakeFiles/xqf.dir/src/tga/memtopixmap.c.o
[100%] Building C object CMakeFiles/xqf.dir/src/tga/tga.c.o
Linking C executable xqf
[100%] Built target xqf
[illwieckz@ubuntu:/home/illwieckz/dev/xqf-skybon/build] cmake ± fakeroot checkinstall --install=no --fstrans --default --pkgname=xqf --pkgversion="1.0.6.2~git" make install

checkinstall 1.6.2, Copyright 2009 Felipe Eduardo Sanchez Diaz Duran
           This software is released under the GNU GPL.


The package documentation directory ./doc-pak does not exist. 
Should I create a default set of package docs?  [y]: y

Preparing package documentation...OK

*** No known documentation files were found. The new package 
*** won't include a documentation directory.

*****************************************
**** Debian package creation selected ***
*****************************************

This package will be built according to these values: 

0 -  Maintainer: [ illwieckz@ubuntu ]
1 -  Summary: [ Package created with checkinstall 1.6.2 ]
2 -  Name:    [ xqf ]
3 -  Version: [ 1.0.6.2~git ]
4 -  Release: [ 1 ]
5 -  License: [ GPL ]
6 -  Group:   [ checkinstall ]
7 -  Architecture: [ amd64 ]
8 -  Source location: [ build ]
9 -  Alternate source location: [  ]
10 - Requires: [  ]
11 - Provides: [ xqf ]
12 - Conflicts: [  ]
13 - Replaces: [  ]

Enter a number to change any of them or press ENTER to continue: 

Installing with make install...

========================= Installation results ===========================
make[2]: *** No rule to make target '../src/gamesxml2c.c', needed by 'CMakeFiles/gamesxml2c.dir/src/gamesxml2c.c.o'. Arrêt.
CMakeFiles/Makefile2:60: recipe for target 'CMakeFiles/gamesxml2c.dir/all' failed
make[1]: *** [CMakeFiles/gamesxml2c.dir/all] Error 2
Makefile:117: recipe for target 'all' failed
make: *** [all] Error 2

****  Installation failed. Aborting package creation.

Cleaning up...OK

Bye.

It seems that make install tries to build gamesxml2c and fails, but whatever the failure cause, make install don't have to build gamesxml2c.

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 21, 2015

Works as intended.

Gamesxml2c is a target on its own. After compilation it processes the games.xml file and is discarded afterwards. make install returns no error.

@illwieckz
Copy link
Member

Hmmm, yesterday it worked with checkinstall, not today, what happened… 😕

@illwieckz
Copy link
Member

Hmm, it's broken with the checkinstall's --fstrans option (that allows people to build a package without superuser permission when using fakeroot).

@illwieckz
Copy link
Member

Wow, something just recreated my whole ~/dev repository with an empty one, hopefully it was only a symlink, checkinstall is not safe at all. o.O

sudo checkinstall --install=yes --default --pkgname=xqf --pkgversion="1.0.6.2~git" make install works, so, it's not an XQF issue.

illwieckz added a commit that referenced this pull request Jun 21, 2015
Goodbye Autotools, hello CMake, thanks @skybon, fix #138, fix # 141, fix #148, fix #155, fix #157, ref #14, ref 129, ref #156
@illwieckz illwieckz merged commit d0b3e59 into XQF:master Jun 21, 2015
@vorot93 vorot93 deleted the cmake branch June 21, 2015 14:58
@lnussel
Copy link
Contributor

lnussel commented Jun 22, 2015

Kids, autotools do support out of tree builds just fine. What made xqf's build scripts complicated was the gettext integrating which was rather suboptimal ten years ago. That's not autotools fault per se. It's fine if you prefer the strange cmake language but please don't blame everything you don't understand on autotools.

@illwieckz
Copy link
Member

Hi @lnussel, glad to see you!

I will not hide that I am not an autotool expert (and not a cmake expert, by the way), my goal and my interest is to continue to maintain XQF and to be able to do that, so anything that simplifies the procedure is welcome. If someone comes with a better autotools based build system or a better cmake based build system, it's ok if it's better than before.

What I see is that I can understand the work done by @skybon, and that's not bad at all. 😉

If you have some suggestions in the future, feel free to talk about them before we merge some code.

I said to @skybon that he must simplify the work of each others, it's the merge condition. So if in this cmake stuff you need something specifically to help you build OpenSUSE packages, please ask it to skybon. If something needs to be improved in the build system, he is now the one to ask.

It's true there is many thing to clean up in this source tree, maybe I do not understand everything, but some things can not be kept as is if we want to move forward. 😄

Yeah, perhaps we are kids, but the project is alive and @skybon is doing good stuff (GTK3 is coming), kids are life. 😀

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 22, 2015

@lnussel Maybe I am not the best expert on Autotools and maybe it's possible to implement the same stuff with it. I don't know.

However what I do know is that Autotools will give you much worse headache than a modern build system. And as I said, build system's main task is ease burden on developers.

Feel free to rewrite old Autotools build system and prove me wrong. And no copy-pasted configure.ac / autogen.sh please. :P

@illwieckz
Copy link
Member

@skybon, just a tip to work in team successfully:

Feel free to rewrite old Autotools build system and prove me wrong.

  • If you want to keep your place in a team, the best thing to do is to not provoke others in duels. Members of the same team never fight each others, they work together.

It's true in real life too.

@illwieckz
Copy link
Member

  • Nobody needs to have two concurrent build system, so it's useless to invite someone to rebuild another one for free and uselessly if the only reason is ego.
  • First @lnussel's commit was 2001-09-27 12:10:34, so there are certainly duels that you will lose. Do not cross swords first. Strategically, this is a mistake, and it is useless.

@illwieckz
Copy link
Member

This PR also fixes #141.

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 27, 2015

We probably got off the wrong foot, so I'm sorry and I concede.

CMake can make things happen, however, and I'm looking forward to us not worrying about build process anymore.

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

Successfully merging this pull request may close these issues.

Kill Autotools with fire
5 participants