Created attachment 338765 [details] [review]
Read DB_CFLAGS and DB_LIBS from cmake options instead of environment variables

Unlike the configure script generated by autotools, cmake doesn't allow
setting environment variables on its command line. This means we can no
longer use environment variables as the only way to specify required
settings because adding environment variables is not supported in
JHBuild modulesets.

Besides, autotools automatically caches used variables in generated
makefiles, while cmake never caches environment variables. As these
project-specific environment variables may be unset most of the time
and re-configuration can be triggered without user confirmation, values
specified with environment variables are likely to lose when cmake is
re-run at unexpected time.

This commit also makes libdb detection failure be fatal, so the build
will stop immediately after failing the test, preventing unexpected
compiling or linking error from happening.

Created attachment 338766 [details] [review]
Add several versioned names of db_load

Some distributions don't have a default version of Berkeley DB and the
name of db_load is always versioned. We only add names for version 5 to
the list because most distributions use that version as their default.

Created attachment 338767 [details] [review]
Unset HAVE_KRB5 before running another CHECK_C_SOURCE_COMPILES test

CHECK_C_SOURCE_COMPILES does nothing if the result variable is already
defined, so we must unset the variable from the cache if we want to
reuse the same variable name.

Review of attachment 338765 [details] [review]:

Thanks for a bug report and patches. If we are going away from the environment variables, then I'll call the options WITH_LIBDB_CFLAGS and WITH_LIBDB_LIBS, simply because there are too many databases (the 'DB' prefix is too ambiguous). I wanted to keep he behaviour as close to the autotools as possible, but I didn't think of the environment variables being such a hassle.

The other patches look fine. Do you want me to do the change, or you'll do it? If you'll do, please merge the patches into one. Thanks in advance.

Created attachment 338834 [details] [review]
Bug 773657 - Fix JHBuild issues on FreeBSD after switching to cmake

I think making it possible to specify BDB options with -D arguments is important because we need it to work in JHBuild. I will update the JHBuild moduleset after the patch gets accepted and committed.

Review of attachment 338834 [details] [review]:

Thanks for the update. It looks fine, except of some typos in the error message. The other comments are for discussion and have nothing to do with this change (except I noticed it when reading the change, and I'd like to know your opinion on it).

::: CMakeLists.txt
@@ +461,2 @@
 if(NOT ("${WITH_LIBDB}" STREQUAL "NO"))
 	if(NOT (("${WITH_LIBDB}" STREQUAL "") OR ("${WITH_LIBDB}" STREQUAL "YES")))

This is nothing with your change, that is fine, I only noticed an inconsistency I caused in this code path. First of all, I claim everywhere "ON" and "OFF" values, but it's checked here for "YES" and "NO", which I think is wrong. Right?

@@ +473,3 @@
 	endif(NOT (("${WITH_LIBDB}" STREQUAL "") OR ("${WITH_LIBDB}" STREQUAL "YES")))
 
 	set(CMAKE_REQUIRED_FLAGS ${LIBDB_CFLAGS})

Is the above line correct? In the light of your other proposals, I'd say it should read:
   set(CMAKE_REQUIRED_INCLUDE_DIRS ${LIBDB_CFLAGS})
instead.

@@ +481,3 @@
+
+	if(NOT HAVE_LIBDB)
+		message(FATAL_ERROR "libdb not found. Use -DWITH_LIBDB=PATH to specify to specify the library prefix, or use -DDB_CFLAGS=-I/path/to/db/include and -DDB_LIBS=/path/to/db/lib to specify arguments for compiling and linking. If you want to disable libdb, please use -DWITH_LIBDB=OFF.")

Here are multiple typos - easy to fix

(In reply to Ting-Wei Lan from comment #7)
> I think making it possible to specify BDB options with -D arguments is
> important because we need it to work in JHBuild.

Sure, I've no problem with the change (I hope it didn't sound like that).

I fixed the typos and committed your change as noted below. I corrected "if(NOT ("${WITH_LIBDB}" STREQUAL "NO"))" and I left the "set(CMAKE_REQUIRED_FLAGS ${LIBDB_CFLAGS})" as it is, that in the second commit.

Created commit b54653c in eds master (3.23.2+)
Created commit dff0abc in eds master (3.23.2+)

(In reply to Milan Crha from comment #8)
> Review of attachment 338834 [details] [review] [review]:
> 
> Thanks for the update. It looks fine, except of some typos in the error
> message. The other comments are for discussion and have nothing to do with
> this change (except I noticed it when reading the change, and I'd like to
> know your opinion on it).
> 
> ::: CMakeLists.txt
> @@ +461,2 @@
>  if(NOT ("${WITH_LIBDB}" STREQUAL "NO"))
>  	if(NOT (("${WITH_LIBDB}" STREQUAL "") OR ("${WITH_LIBDB}" STREQUAL "YES")))
> 
> This is nothing with your change, that is fine, I only noticed an
> inconsistency I caused in this code path. First of all, I claim everywhere
> "ON" and "OFF" values, but it's checked here for "YES" and "NO", which I
> think is wrong. Right?

Yes, you are right. I didn't find this because I never tried to disable it.

> 
> @@ +473,3 @@
>  	endif(NOT (("${WITH_LIBDB}" STREQUAL "") OR ("${WITH_LIBDB}" STREQUAL
> "YES")))
>  
>  	set(CMAKE_REQUIRED_FLAGS ${LIBDB_CFLAGS})
> 
> Is the above line correct? In the light of your other proposals, I'd say it
> should read:
>    set(CMAKE_REQUIRED_INCLUDE_DIRS ${LIBDB_CFLAGS})
> instead.

I didn't notice this because this patch was made before I started to debug the -D problem. (I have to fix evolution-data-server problems before I can try to build evolution.) The problem reported in that bug report is caused by specifying -D option with a list. If arguments are separated by spaces instead of semicolons, it works fine, so I think this doesn't cause problem here because users never pass CFLAGS as a list.

(In reply to Milan Crha from comment #10)
> I fixed the typos and committed your change as noted below. I corrected
> "if(NOT ("${WITH_LIBDB}" STREQUAL "NO"))" and I left the
> "set(CMAKE_REQUIRED_FLAGS ${LIBDB_CFLAGS})" as it is, that in the second
> commit.
> 
> Created commit b54653c in eds master (3.23.2+)
> Created commit dff0abc in eds master (3.23.2+)

Thanks for fixing and committing these patches.