Issue 8878: Adds a Makefile to demonstrate building Google Test with a manually-written Makefile.
Patch Set 1 #Patch Set 2 : Addresses Benoit's comments. #Patch Set 3 : Changes CFLAGS to CXXFLAGS to be consistent with CXX. #Patch Set 4 : Addresses Chandler's comments. #Patch Set 5 : Adds more comments to Makefile. #MessagesTotal messages: 9
http://codereview.appspot.com/8878/diff/1/3 File make/Makefile (right): http://codereview.appspot.com/8878/diff/1/3#newcode21 Line 21: CC = g++ 'c++' is a good default value for most systems, including GNU/Linux, so you may wanna change this. CC is not a good choice since it's most commonly used for the C Compiler. Please use CXX instead. http://codereview.appspot.com/8878/diff/1/3#newcode27 Line 27: # AR = ar Why leave this one commented?
Sign in to reply to this message.
Please take another look? Thanks. http://codereview.appspot.com/8878/diff/1/3 File make/Makefile (right): http://codereview.appspot.com/8878/diff/1/3#newcode21 Line 21: CC = g++ On 2008/12/02 01:03:20, tsuna wrote: > 'c++' is a good default value for most systems, including GNU/Linux, so you may > wanna change this. > > CC is not a good choice since it's most commonly used for the C Compiler. > Please use CXX instead. I switched to CXX. I also changed the default value to c++ and left it commented out, as I trust the default provided by 'make' more. http://codereview.appspot.com/8878/diff/1/3#newcode27 Line 27: # AR = ar On 2008/12/02 01:03:20, tsuna wrote: > Why leave this one commented? The default provided by 'make' is more likely to work.
Sign in to reply to this message.
I've made lots of comments, basically trying to get more minimal and transparent for the whole thing. That said, I'm beginning to question this approach from a different direction. Even with a boring make file, we lose the header inclusion dependency analysis, and end up with a fragile, high-maintenance file; or a very unpredictable file using globbing. So, why have a build system at all? Would converting GoogleTest to a header-only system (albeit, quite a complex header-only system) be feasible? What impediments would there be? http://codereview.appspot.com/8878/diff/204/6 File make/Makefile (right): http://codereview.appspot.com/8878/diff/204/6#newcode14 Line 14: # Points to the root of Google Test, relative to where this file is. Let's simplify this. Put the file at the root of the tree, and call it "foo.make". We can then invoke it with "make -f foo.make", and it won't collide with any autotools. Ideas for "foo" component: basic, baseline, barebones, minimal http://codereview.appspot.com/8878/diff/204/6#newcode20 Line 20: # Name of the C++ compiler. Can we simply omit this entirely? This is well documented Make behavior, and the user can and should read the those documents to know how to override the defaults. http://codereview.appspot.com/8878/diff/204/6#newcode24 Line 24: CFLAGS += -g -I$(GTEST_DIR) -I$(GTEST_DIR)/include The "-g" should be in CXXFLAGS for CXX compiles, and the "-I"s should be in CPPFLAGS. http://codereview.appspot.com/8878/diff/204/6#newcode33 Line 33: # created to the list. Can we word this as "this is a sample test to show how to build your own tests"? If people are using or reading this, they neither want to build the sample, nor add their tests to this make file, but rather add this makefile to their top-level make file, where there tests hopefully live. http://codereview.appspot.com/8878/diff/204/6#newcode38 Line 38: GTEST_HEADER = $(GTEST_DIR)/include/gtest/*.h \ I strongly oppose globbing. It means there cannot ever exist a file which shouldn't be used in a particular build, and generally reduces the explicitness of the build. (I do realize this is mitigated by only using it for the purpose of dependency calculations, I just still think its not optimal.) http://codereview.appspot.com/8878/diff/204/6#newcode54 Line 54: gtest-all.o : $(GTEST_SRCS_) Is there a particular reason to use the single file approach here? I'm assuming simplicity. To that end, I don't know that it's worth build archives below, the .o file links just as easily, or easier due to the lack of 'ar' below. However, if you're interested in more explicitly defining the dependency structure, breaking out each .cc file would help make it simpler. http://codereview.appspot.com/8878/diff/204/6#newcode55 Line 55: $(CXX) $(CFLAGS) -c $(GTEST_DIR)/src/gtest-all.cc There is no need to specify commands for build .o files from .cc files; make will do this automatically. http://codereview.appspot.com/8878/diff/204/6#newcode57 Line 57: gtest_main.o : $(GTEST_SRCS_) Why does this depend on the source files at all? It should just depend on the gtest_main source file and any headers it includes. http://codereview.appspot.com/8878/diff/204/6#newcode61 Line 61: $(AR) $(ARFLAGS) $@ $^ I think you can omit these commands if you decide to keep the archives, although I don't think you need to. http://codereview.appspot.com/8878/diff/204/6#newcode63 Line 63: gtest_main.a : gtest-all.o gtest_main.o I think its reasonable to have all tests link gtest-all, and add gtest_main for the tests which they want the automatic main function. That said, this raises something I've always wondered -- Is there no way to weakly link a main function such that any they define will override ours, and roll it in the basic gtest? http://codereview.appspot.com/8878/diff/204/6#newcode71 Line 71: $(CXX) $(CFLAGS) -c $(USER_DIR)/sample1.cc No command needed. http://codereview.appspot.com/8878/diff/204/6#newcode75 Line 75: $(CXX) $(CFLAGS) -c $(USER_DIR)/sample1_unittest.cc Ditto. http://codereview.appspot.com/8878/diff/204/6#newcode78 Line 78: $(CXX) $(CFLAGS) $^ -o $@ And ditto.
Sign in to reply to this message.
I've uploaded another snapshot. Please take another look. Thanks! Regarding your questions: <quote> That said, I'm beginning to question this approach from a different direction. Even with a boring make file, we lose the header inclusion dependency analysis, and end up with a fragile, high-maintenance file; or a very unpredictable file using globbing. So, why have a build system at all? Would converting GoogleTest to a header-only system (albeit, quite a complex header-only system) be feasible? What impediments would there be? </quote> This Makefile is a demo. Its main purpose is to provide a concrete example on how to build gtest and use it in user tests. Once the user understands how it works, he's free to make the dependency analysis as precise or as conservative as he wants. It's no longer our business. It is not feasible to convert gtest to header-only. That's too big a change, and more importantly, will involve many tricks that make gtest much harder to maintain. Plus, even if it were feasible today, it may not be feasible tomorrow, when some new feature requires a .cc file. We cannot paint ourselves into a corner by committing to a header-only system now. http://codereview.appspot.com/8878/diff/204/6 File make/Makefile (right): http://codereview.appspot.com/8878/diff/204/6#newcode14 Line 14: # Points to the root of Google Test, relative to where this file is. On 2008/12/02 07:55:11, chandlerc wrote: > Let's simplify this. Put the file at the root of the tree, and call it > "foo.make". We can then invoke it with "make -f foo.make", and it won't collide > with any autotools. Ideas for "foo" component: basic, baseline, barebones, > minimal This file is meant as a template for the user to tweak. The user is more likely to want it to be in his own project source tree instead of the Google Test source tree. In the Makefile for Google Mock, we need to tell the compiler whether Google Mock and Google Test live respectively. Such a variable is even more useful there. Therefore I think this flexibility is necessary. Plus, it's tedious to have to type "-f foo.make". http://codereview.appspot.com/8878/diff/204/6#newcode20 Line 20: # Name of the C++ compiler. On 2008/12/02 07:55:11, chandlerc wrote: > Can we simply omit this entirely? This is well documented Make behavior, and the > user can and should read the those documents to know how to override the > defaults. Done. http://codereview.appspot.com/8878/diff/204/6#newcode24 Line 24: CFLAGS += -g -I$(GTEST_DIR) -I$(GTEST_DIR)/include On 2008/12/02 07:55:11, chandlerc wrote: > The "-g" should be in CXXFLAGS for CXX compiles, and the "-I"s should be in > CPPFLAGS. Done. http://codereview.appspot.com/8878/diff/204/6#newcode33 Line 33: # created to the list. On 2008/12/02 07:55:11, chandlerc wrote: > Can we word this as "this is a sample test to show how to build your own tests"? > If people are using or reading this, they neither want to build the sample, nor > add their tests to this make file, but rather add this makefile to their > top-level make file, where there tests hopefully live. My thinking is to let the user create his top-level Makefile from this. Do you think it's better to have a separate top-level Makefile that includes this? Won't that cause all sorts of clashes and mess up the meaning of relative paths? http://codereview.appspot.com/8878/diff/204/6#newcode38 Line 38: GTEST_HEADER = $(GTEST_DIR)/include/gtest/*.h \ On 2008/12/02 07:55:11, chandlerc wrote: > I strongly oppose globbing. It means there cannot ever exist a file which > shouldn't be used in a particular build, and generally reduces the explicitness > of the build. (I do realize this is mitigated by only using it for the purpose > of dependency calculations, I just still think its not optimal.) The top goal of this file is to be simple and easy to understand. It's meant as a starting point that people can easily extend and optimize. I don't want to mix in optimizations to obscure the main message. I'm not concerned with the efficiency of the build, as gtest compiles very fast and its headers/sources don't change for a normal user except when he chooses to upgrade. If a user is concerned with this, he's free to make this more precise. http://codereview.appspot.com/8878/diff/204/6#newcode54 Line 54: gtest-all.o : $(GTEST_SRCS_) On 2008/12/02 07:55:11, chandlerc wrote: > Is there a particular reason to use the single file approach here? I'm assuming > simplicity. To avoid depending on gtest's implementation details. We don't want the user to have to update his tweaked Makefile when gtest gains an additional .cc file, for example. > To that end, I don't know that it's worth build archives below, the > .o file links just as easily, or easier due to the lack of 'ar' below. I want to be consistent with gtest_main.a. Also, a .o file feels more "intermediate" than a .a file. When you define a .a file, there's an understanding that it provides some public service that other targets can use. > However, if you're interested in more explicitly defining the dependency > structure, breaking out each .cc file would help make it simpler. I'm not interested in a precise dependency spec. That is tedious to maintain and bloats the Makefile, obscuring our main message. A conservative but simple dependency spec is better for our purpose. http://codereview.appspot.com/8878/diff/204/6#newcode55 Line 55: $(CXX) $(CFLAGS) -c $(GTEST_DIR)/src/gtest-all.cc On 2008/12/02 07:55:11, chandlerc wrote: > There is no need to specify commands for build .o files from .cc files; make > will do this automatically. I think 'make' can figure it out if the .o and the source share the same base name (and perhaps in the same directory?). In this case, GTEST_SRCS contains multiple .cc files and we only want to compile one. I don't think I can use the default behavior. http://codereview.appspot.com/8878/diff/204/6#newcode57 Line 57: gtest_main.o : $(GTEST_SRCS_) On 2008/12/02 07:55:11, chandlerc wrote: > Why does this depend on the source files at all? It should just depend on the > gtest_main source file and any headers it includes. I'm aiming at simplicity here. gtest sources don't usually change for a normal user, so this doesn't matter much. http://codereview.appspot.com/8878/diff/204/6#newcode61 Line 61: $(AR) $(ARFLAGS) $@ $^ On 2008/12/02 07:55:11, chandlerc wrote: > I think you can omit these commands if you decide to keep the archives, although > I don't think you need to. I want to be consistent with the gtest_main.a rule, which requires the command as it has two source files. Having this command explicitly here also makes it easier for a lay person to understand how it works, although this is of lesser importance. http://codereview.appspot.com/8878/diff/204/6#newcode63 Line 63: gtest_main.a : gtest-all.o gtest_main.o On 2008/12/02 07:55:11, chandlerc wrote: > I think its reasonable to have all tests link gtest-all, and add gtest_main for > the tests which they want the automatic main function. My experience is that in the majority case, the user wants to use main() defined in gtest_main. I want to make the common case easy. > That said, this raises something I've always wondered -- Is there no way to > weakly link a main function such that any they define will override ours, and > roll it in the basic gtest? This can be worked out with gcc, but it's not portable. Plus, I want the user to make a concious choice on whether he wants to use the default main(). If he chooses to use the default main(), we should generate an error if he accidentally links in a .o that contains a main(). http://codereview.appspot.com/8878/diff/204/6#newcode71 Line 71: $(CXX) $(CFLAGS) -c $(USER_DIR)/sample1.cc On 2008/12/02 07:55:11, chandlerc wrote: > No command needed. I tried. 'make' cannot figure out how to build sample1.o without the command here. http://codereview.appspot.com/8878/diff/204/6#newcode75 Line 75: $(CXX) $(CFLAGS) -c $(USER_DIR)/sample1_unittest.cc On 2008/12/02 07:55:11, chandlerc wrote: > Ditto. The same. http://codereview.appspot.com/8878/diff/204/6#newcode78 Line 78: $(CXX) $(CFLAGS) $^ -o $@ On 2008/12/02 07:55:11, chandlerc wrote: > And ditto. The same.
Sign in to reply to this message.
LGTM++ I agree with the comments made by Zhanyong. Let's keep this one simple, stupid. It's just a quick and easy way to build the whole thing with a straightforward Makefile. http://codereview.appspot.com/8878/diff/213/14 File make/Makefile (right): http://codereview.appspot.com/8878/diff/213/14#newcode24 Line 24: CXXFLAGS += -g Actually you can probably drop this line too. It's not a *requirement* and I expect that anyone who wants to do a debug build will know how to do it.
Sign in to reply to this message.
Such a tiny limitation as paths, and poof, all the niceness of Make goes away. =/ Thanks for bearing with all those ideas to simplify, even though none worked! =] http://codereview.appspot.com/8878/diff/204/6 File make/Makefile (right): http://codereview.appspot.com/8878/diff/204/6#newcode33 Line 33: # created to the list. On 2008/12/02 08:50:29, Zhanyong wrote: > On 2008/12/02 07:55:11, chandlerc wrote: > > Can we word this as "this is a sample test to show how to build your own > tests"? > > If people are using or reading this, they neither want to build the sample, > nor > > add their tests to this make file, but rather add this makefile to their > > top-level make file, where there tests hopefully live. > > My thinking is to let the user create his top-level Makefile from this. Do you > think it's better to have a separate top-level Makefile that includes this? > Won't that cause all sorts of clashes and mess up the meaning of relative paths? You can (unfortunately) do recursive Make, and most people do (again unfortunately in my book). That is why putting at the top level could be useful, it would allow them to hook into it with a single line, and still allow them to include it (assuming GNU make, or other sufficiently advanced implementation) into their make file. Either way is fine though. > > http://codereview.appspot.com/8878/diff/204/6#newcode38 Line 38: GTEST_HEADER = $(GTEST_DIR)/include/gtest/*.h \ On 2008/12/02 08:50:29, Zhanyong wrote: > On 2008/12/02 07:55:11, chandlerc wrote: > > I strongly oppose globbing. It means there cannot ever exist a file which > > shouldn't be used in a particular build, and generally reduces the > explicitness > > of the build. (I do realize this is mitigated by only using it for the purpose > > of dependency calculations, I just still think its not optimal.) > > The top goal of this file is to be simple and easy to understand. It's meant as > a starting point that people can easily extend and optimize. I don't want to > mix in optimizations to obscure the main message. > > I'm not concerned with the efficiency of the build, as gtest compiles very fast > and its headers/sources don't change for a normal user except when he chooses to > upgrade. If a user is concerned with this, he's free to make this more precise. > I like this argument, although I would say it's an argument for the big variables of all files, rather than globbing per se. However, yet another manual list is indeed cumbersome until we have some generative ability in at least one of the build systems to manage this madness for us. We can revisit it at that point maybe? http://codereview.appspot.com/8878/diff/204/6#newcode54 Line 54: gtest-all.o : $(GTEST_SRCS_) On 2008/12/02 08:50:29, Zhanyong wrote: > On 2008/12/02 07:55:11, chandlerc wrote: > > Is there a particular reason to use the single file approach here? I'm > assuming > > simplicity. > > To avoid depending on gtest's implementation details. We don't want the user to > have to update his tweaked Makefile when gtest gains an additional .cc file, for > example. > > > To that end, I don't know that it's worth build archives below, the > > .o file links just as easily, or easier due to the lack of 'ar' below. > > I want to be consistent with gtest_main.a. Also, a .o file feels more > "intermediate" than a .a file. When you define a .a file, there's an > understanding that it provides some public service that other targets can use. If the user is going to build their build system out of this one, it just doesn't seem to provide benefit and costs a layer of abstraction. But its a minor point, I'm fine keeping the archives. > > > However, if you're interested in more explicitly defining the dependency > > structure, breaking out each .cc file would help make it simpler. > > I'm not interested in a precise dependency spec. That is tedious to maintain > and bloats the Makefile, obscuring our main message. A conservative but simple > dependency spec is better for our purpose. > http://codereview.appspot.com/8878/diff/204/6#newcode55 Line 55: $(CXX) $(CFLAGS) -c $(GTEST_DIR)/src/gtest-all.cc On 2008/12/02 08:50:29, Zhanyong wrote: > On 2008/12/02 07:55:11, chandlerc wrote: > > There is no need to specify commands for build .o files from .cc files; make > > will do this automatically. > > I think 'make' can figure it out if the .o and the source share the same base > name (and perhaps in the same directory?). In this case, GTEST_SRCS contains > multiple .cc files and we only want to compile one. I don't think I can use the > default behavior. My impression was that for .o files it did "the right thing" and assumed everything else in the deps was just a dependency, not an argument to the compile step. You would just need to put the gtest-all.cc as the first dep in the list (repetitions are fine). That said, i *don't* know if it'll pick up the paths based on that first dependency, so it may still not work. > > http://codereview.appspot.com/8878/diff/204/6#newcode61 Line 61: $(AR) $(ARFLAGS) $@ $^ On 2008/12/02 08:50:29, Zhanyong wrote: > On 2008/12/02 07:55:11, chandlerc wrote: > > I think you can omit these commands if you decide to keep the archives, > although > > I don't think you need to. > > I want to be consistent with the gtest_main.a rule, which requires the command > as it has two source files. Having this command explicitly here also makes it > easier for a lay person to understand how it works, although this is of lesser > importance. You're call, I'll buy the desire for explicit building of the archive, but the automatic command *does* i think pass all of the dependencies in... I know that the LD variant does, but archives are weird so you may be correct. http://codereview.appspot.com/8878/diff/204/6#newcode71 Line 71: $(CXX) $(CFLAGS) -c $(USER_DIR)/sample1.cc On 2008/12/02 08:50:29, Zhanyong wrote: > On 2008/12/02 07:55:11, chandlerc wrote: > > No command needed. > > I tried. 'make' cannot figure out how to build sample1.o without the command > here. Fun. Seems it can't figure stuff out based on directories. Highly annoying. http://codereview.appspot.com/8878/diff/213/14#newcode24 Line 24: CFLAGS += -g -I$(GTEST_DIR) -I$(GTEST_DIR)/include On 2008/12/02 10:08:23, tsuna wrote: > Actually you can probably drop this line too. It's not a *requirement* and I > expect that anyone who wants to do a debug build will know how to do it. Agreed.
Sign in to reply to this message.
Please see my reply below. I uploaded another snapshot with more comments in Makefile, but there's no real change. Is this something you can accept? Thanks. http://codereview.appspot.com/8878/diff/204/6 File make/Makefile (right): http://codereview.appspot.com/8878/diff/204/6#newcode33 Line 33: # created to the list. On 2008/12/02 17:26:06, chandlerc wrote: > You can (unfortunately) do recursive Make, and most people do (again > unfortunately in my book). That is why putting at the top level could be useful, > it would allow them to hook into it with a single line, and still allow them to > include it (assuming GNU make, or other sufficiently advanced implementation) > into their make file. Either way is fine though. Thanks for explaining and being flexible. I read a bit about recursive make and found it not so simple, especially if you need to communicate between the parent Makefile and the child. For our purpose, I prefer the solution to be self-contained. Thus I'll keep it as is. http://codereview.appspot.com/8878/diff/204/6#newcode55 Line 55: $(CXX) $(CFLAGS) -c $(GTEST_DIR)/src/gtest-all.cc On 2008/12/02 17:26:06, chandlerc wrote: > My impression was that for .o files it did "the right thing" and assumed > everything else in the deps was just a dependency, not an argument to the > compile step. You would just need to put the gtest-all.cc as the first dep in > the list (repetitions are fine). That said, i *don't* know if it'll pick up the > paths based on that first dependency, so it may still not work. It's easy to confuse brevity with simplicity, but I'd like to stress that the two are different and don't always agree (even though there tends to be some co-relation between them). Suppose we can make it work to omit the command. It will make the file smaller, and in an expert's eyes will be simpler. However, to a lay person who doesn't have the deep knowledge on how 'make' works, this can be cryptic and misleading - he may get some wrong idea how it actually works. I think being explicit is a small price to pay here. It makes life much easier for the vast majority of the people, while the few experts can still live with it. http://codereview.appspot.com/8878/diff/213/14#newcode24 Line 24: CFLAGS += -g -I$(GTEST_DIR) -I$(GTEST_DIR)/include On 2008/12/02 17:26:06, chandlerc wrote: > On 2008/12/02 10:08:23, tsuna wrote: > > Actually you can probably drop this line too. It's not a *requirement* and I > > expect that anyone who wants to do a debug build will know how to do it. > > Agreed. While this is not strictly necessary, for tests this is a better default (You don't need to ship compiled tests, so the debug info is not a burden or concern. You do, however, want to debug failing tests, and the lack of debug info will make the job much harder.). It is also true that the user can do this himself, but having this as the default means that he does not have to do it himself. It makes a better out-of-the-box experience. Also, even though it's dead obvious to people who are reasonably make-savvy, it can take some time for a lay person to figure out how to tweak the compiler flags. My experience is that there are always users who rely on copy-paste-n-tweak. The less requirement we put on the user's qualification the better. Obviously we need to balance this with keeping the file small. I think the benefit here overweighs the price.
Sign in to reply to this message.
LGTM, Ship it! =] The comments definitely help, and I agree whole heartedly about explicit rules often being easier to understand despite technically unnecessary.
Sign in to reply to this message.
|
