Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My application has two parts -- a client and a server. Within this, there are different versions for different platforms. The user specifies this in the args, i.e. "make PLATFORM=x11".

However, I feel like I'm separating the client and server build operations in a very inefficient way, and that I have too many variables. It works, but I'm not sure how to improve it.

Any suggested improvements would be much appreciated!

CC=gcc
CFLAGS=-c -Wall
LDFLAGS=
BUILD_DIR = ../build/
OBJ_DIR = $(BUILD_DIR)obj/
LIBS=

CLIENT_NAME_DIR = client/
CLIENT_SRC_FILES := $(wildcard $(CLIENT_NAME_DIR)*.c)
CLIENT_PLATFORM_DIR = $(PLATFORM)/
CLIENT_SRC_FILES += $(wildcard $(CLIENT_NAME_DIR)$(CLIENT_PLATFORM_DIR)*.c)
CLIENT_OBJ_DIR = $(OBJ_DIR)$(CLIENT_NAME_DIR)
CLIENT_OBJ_FILES := $(addprefix $(CLIENT_OBJ_DIR),$(notdir $(CLIENT_SRC_FILES:.c=.o)))
CLIENT_LIBS = $(LIBS) -lX11
CLIENT_EXECUTABLE_NAME = outcast-client
CLIENT_EXECUTABLE=$(BUILD_DIR)$(CLIENT_EXECUTABLE_NAME)

SERVER_NAME_DIR = server/
SERVER_SRC_FILES := $(wildcard $(SERVER_NAME_DIR)*.c)
SERVER_OBJ_DIR = $(OBJ_DIR)$(SERVER_NAME_DIR)
SERVER_OBJ_FILES := $(addprefix $(SERVER_OBJ_DIR),$(notdir $(SERVER_SRC_FILES:.c=.o)))
SERVER_LIBS = $(LIBS)
SERVER_EXECUTABLE_NAME = outcast-server
SERVER_EXECUTABLE=$(BUILD_DIR)$(SERVER_EXECUTABLE_NAME)

all: clean client server

client: $(CLIENT_SRC_FILES) $(CLIENT_EXECUTABLE)

server: $(SERVER_SRC_FILES) $(SERVER_EXECUTABLE)

$(CLIENT_EXECUTABLE): $(CLIENT_OBJ_FILES)
    $(CC) $(LDFLAGS) $(CLIENT_OBJ_FILES) -o $@ $(CLIENT_LIBS)


$(CLIENT_OBJ_DIR)%.o: $(CLIENT_NAME_DIR)$(CLIENT_PLATFORM_DIR)%.c
    $(CC) $(CFLAGS) -c -o $@ $<

$(CLIENT_OBJ_DIR)%.o: $(CLIENT_NAME_DIR)%.c
    $(CC) $(CFLAGS) -c -o $@ $<

$(SERVER_EXECUTABLE): $(SERVER_OBJ_FILES)
    $(CC) $(LDFLAGS) $(SERVER_OBJ_FILES) -o $@ $(SERVER_LIBS)


$(SERVER_OBJ_DIR)%.o: $(SERVER_NAME_DIR)%.c
    $(CC) $(CFLAGS) -c -o $@ $<

clean:
    rm -rf $(BUILD_DIR)
    mkdir -p $(CLIENT_OBJ_DIR)
    mkdir -p $(SERVER_OBJ_DIR)
share|improve this question
add comment

3 Answers

You can definitely simplify that:

TARGET          ?= CLIENT

SRC_FILES       := $(wildcard $($(TARGET)_NAME_DIR)*.c)

                               ^^^^^^^^^^^

Here $(TARGET) will expand first (to CLIENT or what is set on the command line) and then $(CLIENT_NAME_DIR) will be expanded.

So you end up with only a few variables specific to the Client/Server and all the other variables are generalized based on that. The same then applies to all your build rules which can be collapsed down into one set of commands (rather than two).

share|improve this answer
    
Thank you! That cleaned it up quite a bit. –  Conner Bryan Nov 2 '12 at 22:39
add comment

Conner, I have a few minor points.

  • Firstly, you have no includes (ie, no -Idir). Do you really have no headers?

  • You should make the non-targets 'PHONY' so make knows they will never exist:

    .PHONY clean
    clean: 
            rm -f ...
    

    Same with all, server and client

  • It is not normal to clean everything on each build - usually you let make decide what is out of date and needs to be rebuilt. For that you need to have all the dependencies declared to make (eg headers)

  • client and server depend upon the build directories and the executables, but not directly upon the sources.

  • the clean target remakes the build directories. To me, these directories should be made when you build the targets:

    $(CLIENT_OBJ_DIR) $(SERVER_OBJ_DIR):
            mkdir -p $@ 
    
    client: $(CLIENT_OBJ_DIR) $(CLIENT_EXECUTABLE)
    
    server: $(SERVER_OBJ_DIR) $(SERVER_EXECUTABLE)
    
  • -Wall does not really give you 'all' warnings. There are many more that are worth enabling.

share|improve this answer
add comment

Not exactly what you are asking for but I would suggest using cmake instead. Some benefits:

  • less code
  • support for a separate build tree
  • avoiding the white-space (tab) bugs that occur easily in a Makefile

First create a file called CMakeLists.txt with this content:

cmake_minimum_required(VERSION 2.8)
project(outcast)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall")
file(GLOB client_sources "client/${PLATFORM}/*.c")
add_executable(outcast-client ${client_sources})
target_link_libraries(outcast-client x11)
install(TARGETS outcast-client RUNTIME DESTINATION bin)
file(GLOB server_sources "server/*.c")
add_executable(outcast-server ${server_sources})
install(TARGETS outcast-server RUNTIME DESTINATION bin)

then build and install your programs outcast-client and outcast-server like this

erik@linux:~$ ls ~/outcast_source
CMakeLists.txt  client/ server/
erik@linux:~$ mkdir /tmp/build
erik@linux:~$ mkdir /tmp/install
erik@linux:~$ cd /tmp/build/
erik@linux:/tmp/build$ CC=gcc cmake -DPLATFORM=X11 -DCMAKE_INSTALL_PREFIX=/tmp/install ~/outcast_source

Instead of finding the source files with a GLOB expression it is probably better to list them one by one. See http://stackoverflow.com/questions/1027247/best-way-to-specify-sourcefiles-in-cmake

CMake has great support for detecting properties of the build system. You could for instance write something like this in your CMakeLists.txt

find_package(X11)
if(${X11_FOUND})
  file(GLOB client_sources "client/X11/*.c")
  add_executable(outcast-client ${client_sources})
  target_link_libraries(outcast-client ${X11_LIBRARIES})
  install(TARGETS outcast-client RUNTIME DESTINATION bin)
endif()
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.