Re: Adds a Makefile to demonstrate building Google Test with a manually-written Makefile.

[Zhanyong Wan]

I changed CFLAGS to CXXFLAGS.  Please take a look at the latest snapshot.  Thanks!

On Mon, Dec 1, 2008 at 5:14 PM, <shi...@gmail.com> wrote:

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.

http://codereview.appspot.com/8878

--
Zhanyong

[Chandler Carruth]

I'm working on some comments, sorry its taking so long.

[chan...@gmail.com]

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.

http://codereview.appspot.com/8878

[shi...@gmail.com]

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.

http://codereview.appspot.com/8878

[chan...@gmail.com]

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.

http://codereview.appspot.com/8878

[shi...@gmail.com]

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.

http://codereview.appspot.com/8878

[chan...@gmail.com]

LGTM, Ship it! =] The comments definitely help, and I agree whole
heartedly about explicit rules often being easier to understand despite
technically unnecessary.

http://codereview.appspot.com/8878