Saturday, November 28, 2015
VerySillyMUD: Continuous Integration

This post is part of my "VerySillyMUD" series, chronicling an attempt to refactor an old MUD into working condition[1].


In our last episode, we got an autotools-driven build set up, which gets us going along the path towards being able to use a continuous integration tool like Travis CI. In this episode, we'll see if we can get it the rest of the way there. I'll be referencing the getting started guide, the custom build instructions, and the C project instructions.


It seems like the easiest way to proceed is to try to just use the default C build, which I expect not to work, but then to massage it into shape by looking at the build errors. It seems like this minimal .travis.yml is a good starting point:




As an editorial comment, the autotools toolchain uses "make check" to run tests, but Travis CI expects you to have autotools and that the default way to run tests is..."make test". I kind of wonder how this happened; my suspicion is that most projects use make test (and so that's what Travis CI assumes) but that GNU autotools defined an opinionated standard of make check that ignored existing practice.


Anyway, back to the build. As expected, this failed because there wasn't a configure script to run! This is interesting--I had not checked in any of the files generated by autoconf or automake under the general principle of "don't check in files that get generated". Ok, we should be able to just run autoreconf to get those going:




This one fails too, with the following error from autoreconf:




Hmm, very mysterious. This seems to be due to missing the AC_CHECK_HEADER_STDBOOL macro, and at least on this mailing list post, it was suggested it could be removed, so let's try that.


Ok, that build got further, and in fact built the main executable, which is great; we just failed when trying to find the Criterion header files. Since we haven't done anything to provide that dependency, this isn't surprising. I also notice that there are a lot more compiler warnings being generated now (this would appear to be based on having a different compiler, gcc vs. clang). Now we just need to decide how to provide this library dependency. The build container is an Ubuntu system, which means it uses apt-get for its package management, but there doesn't seem to be a .deb package for it provided. The options would seem to be:

  • vendor the source code for it into our repository
  • figure out how to build a .deb for it and host it somewhere we can install it via apt-get
  • download a Linux binary distribution of Criterion
  • download the source code on the fly and build it locally

I'm not crazy about vendoring, as that makes it harder to get updates from the upstream project; I'm not crazy about the binary download either, as that may or may not work in a particular environment. My preference would be to build a .deb, although I haven't done that before. I assume it would be similar to building RPMs, which I have done. Downloading and building from source is perhaps a good initial method, as I know I could get that working quickly (I have built the Criterion library from source before). If I ever get tired of waiting for it, I can always revisit and do the .deb.


According to the Travis CI docs, the usual way to install dependencies is to customize the install step with a shell script. We'll try this one to start, based on the instructions for installing Criterion from source:




Ok, this does pretty good as far as starting some of the compilation process, but it still fails with this error:




Hmm, seems to be looking for some copyright information; we'll try copying Criterion's LICENSE file into the debian.copyright file it seems to be looking for. Ok, that build succeeded in building and installing the library, and in fact, the later ./configure found it, but wasn't able to load the shared library libcriterion.so. We need to add an invocation of ldconfig after installing the library, I think. Wow, that did it! We have a passing build!


Let's record our success by linking to the build status from README.md so that people visiting the repo on GitHub know we have our act together!


Now, while debugging this build process, I got several notices that the project was running on Travis CI's legacy infrastructure instead of their newer container-based one, which purports to have faster build start times and more resources, according to their migration guide. It seems like for us the main restriction is not being able to use sudo; we use this in exactly three places at the moment:

  1. to install the check package via apt-get
  2. to run make install after building the Criterion library
  3. to run ldconfig after installing the Criterion library

It seems like there are built-in ways to install standard packages via apt, so then the question is whether we can directly compile and link against the Criterion library from the subdirectory where we downloaded and built it; if we can then we don't need sudo for that either. Ok, it looks like we just need to find the include files in the right place, by adding an appropriate -I flag to CFLAGS and then to find the built shared library by pointing the LD_LIBRARY_PATH environment variable to the right place. Nope. Nope. Nope. Nope. Ok, this answer on StackOverflow suggests we need to pass arguments through to the linker via -W options in LDFLAGS. Still nope. Maybe if we pass the Criterion build directory to the compiler via -L and to the runtime linker via -R? Bingo!


Ok, now we just need to see if we can install the check submodule via the apt configuration directives in .travis.yml. That works, and here's the final working CI configuration:




It seems that by setting CFLAGS explicitly here, we "fixed" the compiler warnings; I suspect we need to come back around and add -Wall to CFLAGS and then fix all those warnings too. But that can wait for the next episode...



[1] SillyMUD was a derivative of DikuMUD, which was originally created by Sebastian Hammer, Michael Seifert, Hans Henrik Stærfeldt, Tom Madsen, and Katja Nyboe.

Friday, November 20, 2015
VerySillyMUD: Adding autotools

This post is part of my "VerySillyMUD" series, chronicling an attempt to refactor an old MUD into working condition[1].

In our last episode, we got all the remaining compiler warnings fixed. While I was merging the pull request for it, I noticed GitHub was prompting me to get continuous integration set up. I had run across Travis CI before, and knew that it was free to use for open source projects. A quick peek around their documentation shows that they support C, but that they assume the use of GNU autotools for building. Since a friend had already identified weirdness from the runs of makedepend I had done on my own computer and checked in, I actually already had an issue open for this. Seems like the universe is trying to tell me something!

Conveniently, autotools comes with a sample project and a pretty good step-by-step tutorial. We also have a working Makefile that we can use for reference--for now I'm just going to make a temporary copy of it as Makefile.orig so that I can have it for easy reference, and then clean it up later during commit/PR structuring. Since automake is going to overwrite the Makefile, this will be convenient, even though I know a copy of the Makefile is safely tucked away in version control. Ok, let's start with the toplevel Makefile.am, which for now just has to point to the src/ directory:

Then we need another Makefile.am for the src/ directory. In this case, it looks like the bare minimum is to identify the final executable name and then identify the .c source files. Not sure if we need to add the .h ones or not yet; it could be that autoconf will find those later. Anyway, let's try this:

As for the confugre.ac in the source directory, we can adapt the sample one from here and try this:

Now, per the instructions, we're supposed to run "autoreconf --install":

Hmm, I had thought that all the CFLAGS would go in the AM_INIT_AUTOMAKE macro, but I guess not. Let's just put -Werror in there for now and try again:

Ok, this is much closer. Looks like we just have some missing files. For now, I'll create empty versions of NEWS, README, AUTHORS, and ChangeLog, and remember to create an issue to fill those in. As for COPYING, that's traditionally the license file, so we'll just make a copy of doc/license.doc and use that. Now when we run `autoreconf --install` it completes successfully! Ok, let's try running ./configure:

Wow, that worked. Ok, let's try doing a make:

Ah, failure. A quick look at the output shows we're missing some CFLAGS; this might be the source of this compilation error, since one of the compilation flags was -DGROUP_NAMES and that might be what the gname field is for. A quick look at the declaration of struct char_special_data in structs.h confirms this is the case. Ok, so we just need to figure out how to get the CFLAGS declared properly. According to this answer on StackOverflow, it seems we can just add them in the configure.ac file.

CFLAGS+=" -DIMPL_SECURITY -DNEW_RENT -DLEVEL_LOSS -DNEWEXP -DGROUP_NAMES"

In the process of looking for advice on adding the CFLAGS, I ran across a description of the autoscan tool that will scan your source code and suggest missing macro invocations for your configure.ac. A quick run shows that we're mostly missing detection of the header files we include and the library functions we call, so we'll just add that in too:

AC_CHECK_HEADERS([arpa/inet.h fcntl.h malloc.h netdb.h netinet/in.h
  stdlib.h string.h strings.h sys/socket.h sys/time.h unistd.h])

AC_CHECK_HEADER_STDBOOL
AC_TYPE_SIZE_T

AC_FUNC_MALLOC
AC_FUNC_REALLOC
AC_CHECK_FUNCS([bzero gethostbyaddr gethostbyname gethostname
  getpagesize gettimeofday inet_ntoa isascii memset select socket
  strchr strdup strrchr strstr])

And...wow! It builds without errors. Now, there's still a few things missing here, like actually using the defined macros in the config.h that the configure script generates; we also haven't gotten the tests running yet, or looked at what "make install" wants to do. So let's get started with the tests. The first thing we're going to want to do is pull the common source code files out into their own variable:

common_sources = comm.c act.comm.c act.info.c act.move.c act.obj1.c \
 act.obj2.c act.off.c act.other.c act.social.c act.wizard.c handler.c \
 db.c interpreter.c utility.c spec_assign.c shop.c limits.c mobact.c \
 fight.c modify.c weather.c spells1.c spells2.c spell_parser.c \
 reception.c constants.c spec_procs.c signals.c board.c magic.c \
 magic2.c skills.c Opinion.c Trap.c magicutils.c multiclass.c hash.c \
 Sound.c Heap.c spec_procs2.c magic3.c security.c spec_procs3.c \
        create.c bsd.c parser.c intrinsics.c
dmserver_SOURCES = $(common_sources) main.c

At this point, while perusing through the automake manual to figure out how to do tests, I discovered there was a better way to define the symbols instead of adding them to CFLAGS in configure.ac; there's an automake variable for this called AM_CFLAGS, so we just move the flags over to Makefile.am instead. But in the meantime, the next step towards tests would be to correctly find the header files and library for Criterion in the configure script, so that the generated Makefile looks for them in the right place. We can do this by adding the following to configure.ac:

AC_CHECK_HEADERS([criterion/criterion.h],[],[
  echo "***WARNING***: unit tests will not be runnable without Criterion library"
  echo "  See https://github.com/Snaipe/Criterion/"
])
AC_CHECK_LIB(criterion,extmatch,[],[
  echo "***WARNING***: unit tests will not be runnable without Criterion library"
  echo "  See https://github.com/Snaipe/Criterion/"
])

After a little more perusing through the autotools manual, it turns out instead of the echo command there's a canonical way to do this using the AC_MSG_WARN macro, as in:

AC_CHECK_HEADERS([criterion/criterion.h],[],[
  AC_MSG_WARN(unit tests will not be runnable without Criterion library)
  AC_MSG_WARN(See https://github.com/Snaipe/Criterion/)
])

Now, when we then run make, we find the Criterion library, but the final dmserver executable gets linked with -lcriterion, which we don't want, because as you may recall, that library has a main() function in it that is going to try to run test suites, so we don't actually want the default action of AC_CHECK_LIB. Instead, we need to fake it out:

AC_CHECK_LIB(criterion,extmatch,[
  AC_MSG_RESULT(yes)
],[
  AC_MSG_WARN(unit tests will not be runnable without Criterion library)
  AC_MSG_WARN(See https://github.com/Snaipe/Criterion/)
])

And now we can go ahead and build the unit test program and indicate that's what should run during "make check" by adding the following to src/Makefile.am:

# unit tests
check_PROGRAMS = tests
tests_SOURCES = $(common_sources) test.act.wizard.c
tests_LDADD = -lcriterion
TESTS = tests

Sure enough, we can run the tests:

Nice. Ok, now these hardcoded AM_CFLAGS are still bothering me. Really, we ought to be able to opt into and out of them via feature enabling/disabling from configure. My friend Dan would probably, at this point, say "Why?" in incredulity, but this is not an exercise in practicality, per se... The way we do that is to add these flags to configure.ac, which will cause configure to output them into config.h. We can do that with stanzas like the following:

AH_TEMPLATE([IMPL_SECURITY],
        [Define as 1 to restrict each level 58+ god to one site or set of
         sites])
AC_ARG_ENABLE([impl-security],
  [AS_HELP_STRING([--disable-impl-security],
    [turn off site-locking for gods])],
  [],
  [AC_DEFINE([IMPL_SECURITY])])

Ok, then we need to just go around and include config.h in all the .c and .h files, and then we can remove the AM_CFLAGS from Makefile.am. Cleaner! At this point, the last thing to do is to get make install to work. It turns out the default action for the MUD server itself is the right one, but we also need to collect the data files and install them. This can be done by creating Makefile.ams in various subdirectories. For example, here's the one for the top-level lib/ directory:

Then the last thing to is to make the compiled server look there by default. The configure script takes a --localstatedir argument to customize where those data directories get created; we really want the game to have that path compiled into it as a default path. After much noodling through StackOverflow and the automake manuals, it looks like the best way to do this is a multi-step process in order to get the right amount of variable expansion. First, we have to bring our friend AM_CFLAGS back and pass the Makefile level variable $(localstatedir) in as a symbol definition:

AM_CFLAGS = -DLOCAL_STATE_DIR=\"$(localstatedir)\"

Then, we can add the following to configure.ac:

AC_DEFINE([DEFAULT_LIBDIR],[LOCAL_STATE_DIR "/sillymud"],
  [Default location to look for game data files])

...which results in the following showing up in config.h:

/* Default location to look for game data files */
#define DEFAULT_LIBDIR LOCAL_STATE_DIR "/sillymud"

This makes whatever got passed in to the --localstatedir argument of configure, which defaults to /usr/local/var, to show up as a string literal LOCAL_STATE_DIR. In C, two string literals adjacent to one another get concatenated, so this results in DEFAULT_LIBDIR being something like /usr/local/var/sillymud. At this point, we're able to run make install and run /usr/local/bin/dmserver. I think for our next episode, it's time to do a little play testing to see how well things are working and what else needs fixing!


[1] SillyMUD was a derivative of DikuMUD, which was originally created by Sebastian Hammer, Michael Seifert, Hans Henrik Stærfeldt, Tom Madsen, and Katja Nyboe.

Saturday, November 14, 2015
VerySillyMUD: Fixing the Rest of the Compiler Warnings

This post is part of my "VerySillyMUD" series, chronicling an attempt to refactor an old MUD into working condition[1].

In our last episode, we used unit tests to document a function's behavior so we could refactor it to fix a security warning. Now we are able to pick up where we left off three episodes ago: fixing compiler warnings to find any gotchas. To get started, let's re-enable the -Werror compiler flag. Now, back to fixing warnings. I'll just highlight the ones here that are novel cases we haven't seen before. First up:

Ok, let's get some context; here's the function where that warning/error appears:

So we see that d is a struct descriptor_data *. A little grepping around shows that's defined in structs.h:

Sure enough, we can see that d->host is a statically-allocated array. Now, this seems like a recipe for a buffer overrun to me, but one thing at a time. We'll maybe find or fix that later by running lint or valgrind. For now, though, I'll create an issue so we don't forget, then we can just fix this to a simpler test and move on.

Well, it turns out that was the most interesting warning/error to highlight. In almost all cases it was pretty clear how to fix the warning, and often the compiler's error message told me exactly how to fix it! Since there were a LOT of changes to get here, we'll spare you the details. At this point, the game starts without most of the warnings we used to see. Hurrah! Now the only question is what to do for the next episode...


[1] SillyMUD was a derivative of DikuMUD, which was originally created by Sebastian Hammer, Michael Seifert, Hans Henrik Stærfeldt, Tom Madsen, and Katja Nyboe.

Sunday, November 8, 2015
VerySillyMUD: Understanding a Function with Tests

This post is part of my "VerySillyMUD" series, chronicling an attempt to refactor an old MUD into working condition[1].

In our last episode, we got a basic framework for unit tests in place. Just to refresh our context (and which layer of yak we are shaving at the moment): we need unit tests so we can (a) capture the current behavior of a particular function, dsearch(), so that we can (b) refactor it safely, so that we can (c) fix a compiler warning about it, so that we can (d) finish fixing all the compiler warnings in case there are other latent bugs being hidden. Whew!

So let's take a look again at the function in question:

What can we figure out about it? Well, first, it seems to take two string arguments, helpfully named "string" and "tmp". It seems like both are potentially modified by the function here; tmp gets written on line 11 via strcpy(), and string gets written on line 24 via sprintf(). The i variable seems to be a flag indicating whether we're done, as the main while loop tests for it and the only modification to it is in line 10 where it is set to 1. So let's start with the simplest case, where we only go through the loop once; in order for that to be true, we need the call to strchr in line 9 to find no instances of the tilde ('~') character in string. In this case, it looks like we just copy string into tmp and are done, which suggests that our first unit test can be:

So let's try to run that:

Now, what about a more complicated test? It seems like tmp might be the output variable here, so let's see what happens when we feed it a string with a tilde in it, by writing an intentionally failing test:

And then run the test:

Hmm, a little less than I was hoping for; the Criterion library macro-interpolates the C source expression into the error message. In the case of string comparison, I'd rather have seen the string values here instead. At this juncture I took a quick browse of the Criterion source code to see if this was a quick fix, but it wasn't, so I decided to pass on this for now and just fix it by doing a good ol' printf to see what the value was. In this case, it looks like the behavior is to strip out the tilde and the following space, because this test passes:

A quick perusal of the source shows we must be in the default: clause of the switch statement. Now, like any good tester, let's see what happens if we have a tilde as the last character:

Ah, cool! That one passes. So now let's look at some of the other cases, starting with a "~N". I'm going to guess that this substitutes the string "$n;" in for "~N":

Indeed, this passes. Similarly, it looks like "~H" will turn into "$s". (It does, although I won't show that test here just for brevity). Ok, what should we test next? How about two tildes in a row? I think that this should just get stripped out because the second tilde won't be either 'N' or 'H':

Yep. Ok, now we have a loop, so this suggests we should be able to do multiple substitutions:

Check. That would seem to cover most of the output behavior on the second argument to dsearch(), which is labeled tmp. As we noted, though, the first parameter, string sometimes also gets written, in the sprintf on line 24. So we had better document its behavior too. However, notice that sprintf(string,tmp) essentially copies tmp into string, as long as there are no format expansions like %d in tmp, and if there are, then we don't have any arguments for them! So this is likely a bug, especially if string comes from an untrusted input source. Now, if you remember a couple episodes ago, this was the exact compiler warning we ran into:

Ok, so this means that we should pretty much just add tests that show that string has the same output behavior as tmp, and then I think we can refactor it.

At this point I feel pretty confident I understand what this function does. Let's rewrite it more simply. What we'll do is we'll build up the substituted string in tmp and then copy it back into string at the end:

Sure enough, the tests pass, so we'll call this a victory for now. In the next episode, we'll turn the -Werror flag back on and continue fixing compiler warnings as we were trying to do previously. Fortunately, we now have the beginnings of some unit tests we can use if we encounter anything that doesn't look like it has an immediately obvious fix.


[1] SillyMUD was a derivative of DikuMUD, which was originally created by Sebastian Hammer, Michael Seifert, Hans Henrik Stærfeldt, Tom Madsen, and Katja Nyboe.