Update scons, add Travis, qmake build and tests, fix #2 #6

richelbilderbeek · 2019-12-28T10:13:21Z

Hi Globulation 2 maintainers,

As per this Issue I submit this Pull Request.

Changes:

scons now uses makeVariable and Variable instead of the deprecated makeOptions and Options
This is checked by a Travis CI script: upon a commit, the build process is started on a virtual machine, so one can see if a commit breaks a build
The build is also re-done by a Qt Creator project file, src/globulation2.pro
The build is also re-done by a Qt Creator project file, src/globulation2_test.pro that runs the cpptests in the test folder. Although these tests seem rather trivial, my new addition may upon up the path to write more tests in the future

I volunteer to maintain the .travis.yml and .pro(and .pri) files.

Enjoy 👍

richelbilderbeek · 2019-12-28T11:35:13Z

I've also added a Build badge in README.md.

The GitHub user with the name Globulation2 does need to activate Travis CI here (you can log in from GitHub), as it is not activated yet.

stephanemagnenat

Thanks for your contributions!

Your PR currently has a lot of commits, some of them fixing things of previous commits, would it be ok for you to rebase them into a small sets of commits (e.g. one that fixes scons, one that adds Travis, one that adds Qt build, etc.)?

stephanemagnenat · 2019-12-28T15:34:56Z

scripts/update

+#
+
+cd glob2
+hg pull


Why hg, as we transitioned to git?

You are right, It's an old update script ...

I suggest to even delete the script: I used it to sync my repo (https://github.com/richelbilderbeek/globulation2) to use a newer version of this repo. As the code I submitted is part of the repo it works on, this script is obsolete.

Ok, can you do the deletion, so that I do not delete the wrong thing?

stephanemagnenat · 2019-12-28T15:35:58Z

scripts/install_deps

@@ -0,0 +1,10 @@
+#!/bin/bash


Should we somehow name this script in relation with the type of system it installs deps on, such as install_deb_deps?

I agree with the reasoning. Go ahead and rename, you're the boss here.

If you want, I'll rename 👍

Can you rename it please

stephanemagnenat · 2019-12-28T15:41:31Z

I am curious, what is the rationale for the qmake-based build parallel to the scons one?

richelbilderbeek · 2019-12-28T15:43:43Z

I am curious, what is the rationale for the qmake-based build parallel to the scons one?

To show that a scons and qmake build are equally fine.

richelbilderbeek · 2019-12-28T15:47:34Z

Your PR currently has a lot of commits, some of them fixing things of previous commits, would it be ok for you to rebase them into a small sets of commits (e.g. one that fixes scons, one that adds Travis, one that adds Qt build, etc.)?

I would be open to do so, but I have no experience with rebasing (I feel it's not worth it). Due to this, I'd worry I'd mess up things.

If you'd insist, I can just redo all the steps by hand (of course, I prefer to leave things as they are). If so, let me know, and I'll do 👍

Thanks for your review!

stephanemagnenat · 2019-12-28T16:06:34Z

I can do the rebase if you want. Note that it is actually not that spooky, there are a lot of introductions online.

That's great if you maintain the travis and the pro files.

stephanemagnenat · 2019-12-28T16:09:42Z

test/TestsRunner.cpp

@@ -29,6 +29,10 @@
 #include <cppunit/ui/text/TestRunner.h>
 #include <cppunit/XmlOutputter.h>

+//Globulation2 has one global
+#include "GlobalContainer.h"
+GlobalContainer *globalContainer = new GlobalContainer;


Why do we need to create the GlobalContainer here? To see whether it fails? A comment would be helpful!

It resolves an unresolved external error.

I felt a comment like // make it link a bit unhelpful. The comment I did write was a left-over from // Globulation2 has one global :-( (I prefer my locals), which I think is unhelpful as well. Go ahead and remove it 👍

(of course, I'd be happy to remove it myself)

richelbilderbeek · 2019-12-28T16:38:55Z

That's great if you maintain the travis and the pro files.

Just to re-iterate: I'll happily do so 👍

stephanemagnenat · 2019-12-29T11:42:10Z

@randomvoids for me this PR looks ok, if it is ok for you I can rebase to cleanup its messy history and merge.

kylelutze · 2019-12-29T15:29:33Z

I am curious, what is the rationale for the qmake-based build parallel to the scons one?

To show that a scons and qmake build are equally fine.

Is there a reason why we need both though? I would prefer to not have to maintain 2 build systems if we can avoid it. Is qmake better than scons or ?

richelbilderbeek · 2019-12-29T15:42:44Z

@randomvoids:

Is there a reason why we need both though? I would prefer to not have to maintain 2 build systems if we can avoid it. Is qmake better than scons or ?

AFAICS, qmake is just as fine as scons. I used qmake because I am comfortable with it (unlike scons). Thanks to qmake, I could also setup a qmake project to run the tests in the tests folder (something the scons build does not (?) do). Additionally, I can open up the project in my favorite IDE called Qt Creator.

I can imagine the writer of the scons setup prefers scons and volunteers to maintain it, like I would enjoy maintaining the qmake project files.

There are some options that may be considered (and I volunteer to implement each any decision):

I can setup the Travis CI file to either scons or qmake depending on the branch name.
Or perhaps more fun: I can setup the Travis CI file to use two compilers (GCC and clang) and use scons for the first and qmake for the second compiler.

&TLDR; both are equally fine, I am just more comfortable with qmake

stephanemagnenat · 2019-12-30T11:14:01Z

A "problem" with qmake is that it adds a dependency on Qt, which feels a bit wrong just to use its build system. If it is in addition to scons, why not, if it is the main one, I would be careful before changing.

As I said on the ml, I am worried about remorking the build system without a need as that is bikeshedding and there are more important issues to tackle to let the project progress. Changing the build system would also imply redoing documentation, which currently exists in several languages, so getting to where we are now is a lot of work, for no clear added benefits. I am not against having another build system on the side (maybe in a branch?) if it helps people, or switching if there is a good rationale for, but otherwise, I am not fully convinced to switch. Also, switching the build system pushes work on distro packagers which might reduce Glob2's visibility for a while if we switch.

richelbilderbeek · 2019-12-30T12:06:04Z

[...] qmake is that it adds a dependency on Qt, which feels a bit wrong just to use its build system. If it is in addition to scons, why not.

Cool!

You may overlook some benefits though:

Adding a Qt Creator project file (those .pro files) allows people to use the Qt Creator IDE to develop on Globulation 2
As I am comfortable with qmake, there will be more Pull Requests filled with goodies. For example, adding cppcheck to the Travis build. Note that @randomvoids already made an Issue due to an error spotted by cppcheck.

Again: I agree to keep scons the primary build system and I do volunteer to maintain the Travis and .pro/.pri files.

richelbilderbeek · 2020-01-02T09:13:59Z

@stephanemagnenat: how is the rebase going?

stephanemagnenat · 2020-01-03T19:55:37Z

I split this PR into two, the SCons fix is in master, the rest in that branch.

While doing it, I encountered several questions:

What is the purpose of the scripts you added, as if I'm not mistaken they are not used?
There seems to be duplications, for example cppunit is built by autoconf both in .travis.yml and in scripts/install_cppunit, but also by Qt in src/cppunit.pri, I do not understand why. They make the whole system hard to understand because one does not know what is necessary or not.

I would like to address these questions before merging the rest of the PR, because otherwise we import hard-to-maintain stuff that would confuse people entering the project, as they confuse me.

richelbilderbeek · 2020-01-08T12:44:12Z

What is the purpose of the scripts you added, as if I'm not mistaken they are not used?

check_dead_links: checks for dead links within a repo
install_cppunit: installs CppUnit
install_deb_deps: installs the dependencies (the Debian packages, that is)

There seems to be duplications, for example cppunit is built by autoconf both in .travis.yml and in scripts/install_cppunit

This is a temporary duplication: I supplied a stand-alone .travis.yml file. I naively thought that that would give me the path of least resistance getting my Pull Request accepted. Ideally, all three scripts are called from the .travis.yml file, which will help shorten it.

There seems to be duplications, for example cppunit is built [...] in .travis.yml [...], but also by Qt in src/cppunit.pri

Here, there is no duplication: install_cppunit installs CppUnit, cppunit.pri adds the files to the C+ project.

richelbilderbeek · 2020-01-17T15:13:32Z

@stephanemagnenat: what is the status of this PR?

richelbilderbeek · 2020-02-22T10:23:15Z

@stephanemagnenat: what is the status of this PR?

stephanemagnenat · 2020-02-22T10:35:08Z

I think it is up to the maintainer (which is not me) to take the decision on whether to accept an additional build system. I did apply the part fixing Scons in 51ebae2.

richelbilderbeek · 2020-02-22T10:38:41Z

I'll take my loss then, submitting a Pull Request usually is a more fun experience.

richelbilderbeek added 15 commits December 27, 2019 09:35

Add Travis CI build script

31d92f1

Travis clones GitHub (not Bitbucket) repo

4efa2de

Add Qt Creator project files

a387615

Clone glob2 in its own folder

3eb3721

No need to clone the GitHub itself

59ad1d6

Update SConstruct to follow new scons naming

ba9b1d9

Qt Creator project files in src/

104e00d

Use correct Qt Project file path

7cabae6

Only build

97c8a2b

Minimal Travis build

4dc2b3d

Run tests

d29957e

Build with scons

cbbc173

Compile tests with C++11

d04f648

Compile with GCC 9

24428fc

Show build status [skip ci]

c77578b

stephanemagnenat reviewed Dec 28, 2019

View reviewed changes

Remove obsolete 'update' script

b0c2a64

richelbilderbeek and others added 3 commits December 29, 2019 08:54

Rename to 'install_deb_deps' (from 'install_deps'), for Globulation2#6

02cef6a

Renamed 'README' to 'README.md', fix Globulation2#8

f6af0f8

Merge branch 'master' into master

73641ec

richelbilderbeek closed this Feb 22, 2020

Update scons, add Travis, qmake build and tests, fix #2 #6

Update scons, add Travis, qmake build and tests, fix #2 #6

richelbilderbeek commented Dec 28, 2019

richelbilderbeek commented Dec 28, 2019

stephanemagnenat left a comment

stephanemagnenat Dec 28, 2019

richelbilderbeek Dec 28, 2019 •

edited

stephanemagnenat Dec 28, 2019

richelbilderbeek Dec 28, 2019

stephanemagnenat Dec 28, 2019

richelbilderbeek Dec 28, 2019

richelbilderbeek Dec 28, 2019

kylelutze Dec 28, 2019

richelbilderbeek Dec 29, 2019

stephanemagnenat commented Dec 28, 2019

richelbilderbeek commented Dec 28, 2019

richelbilderbeek commented Dec 28, 2019

stephanemagnenat commented Dec 28, 2019

stephanemagnenat Dec 28, 2019

richelbilderbeek Dec 28, 2019

richelbilderbeek Dec 28, 2019

richelbilderbeek commented Dec 28, 2019

stephanemagnenat commented Dec 29, 2019

kylelutze commented Dec 29, 2019

richelbilderbeek commented Dec 29, 2019

stephanemagnenat commented Dec 30, 2019 •

edited

richelbilderbeek commented Dec 30, 2019

richelbilderbeek commented Jan 2, 2020

stephanemagnenat commented Jan 3, 2020 •

edited

richelbilderbeek commented Jan 8, 2020 •

edited

richelbilderbeek commented Jan 17, 2020

richelbilderbeek commented Feb 22, 2020

stephanemagnenat commented Feb 22, 2020 •

edited

richelbilderbeek commented Feb 22, 2020

Update scons, add Travis, qmake build and tests, fix #2 #6

Update scons, add Travis, qmake build and tests, fix #2 #6

Conversation

richelbilderbeek commented Dec 28, 2019

richelbilderbeek commented Dec 28, 2019

stephanemagnenat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richelbilderbeek Dec 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanemagnenat commented Dec 28, 2019

richelbilderbeek commented Dec 28, 2019

richelbilderbeek commented Dec 28, 2019

stephanemagnenat commented Dec 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richelbilderbeek commented Dec 28, 2019

stephanemagnenat commented Dec 29, 2019

kylelutze commented Dec 29, 2019

richelbilderbeek commented Dec 29, 2019

stephanemagnenat commented Dec 30, 2019 • edited

richelbilderbeek commented Dec 30, 2019

richelbilderbeek commented Jan 2, 2020

stephanemagnenat commented Jan 3, 2020 • edited

richelbilderbeek commented Jan 8, 2020 • edited

richelbilderbeek commented Jan 17, 2020

richelbilderbeek commented Feb 22, 2020

stephanemagnenat commented Feb 22, 2020 • edited

richelbilderbeek commented Feb 22, 2020

richelbilderbeek Dec 28, 2019 •

edited

stephanemagnenat commented Dec 30, 2019 •

edited

stephanemagnenat commented Jan 3, 2020 •

edited

richelbilderbeek commented Jan 8, 2020 •

edited

stephanemagnenat commented Feb 22, 2020 •

edited