Hi Emil,
From: linux-media-owner@vger.kernel.org [mailto:linux-media- owner@vger.kernel.org] On Behalf Of Emil Velikov Sent: Wednesday, April 29, 2015 5:00 PM
Hi Kamil,
Allow me to put a few suggestions:
On 29 April 2015 at 11:02, Kamil Debski k.debski@samsung.com wrote:
This is the first version of the libGenCEC library. It was designed
to
act as an interface between the generic CEC kernel API and userspace applications. It provides a simple interface for applications and an example application that can be used to test the CEC functionality.
signed-off-by: Kamil Debski k.debski@samsung.com
AUTHORS | 1 + INSTALL | 9 + LICENSE | 202 ++++++++++++++++ Makefile.am | 4 + README | 22 ++ configure.ac | 24 ++ doc/index.html | 345 +++++++++++++++++++++++++++ examples/Makefile.am | 4 + examples/cectest.c | 631
++++++++++++++++++++++++++++++++++++++++++++++++++
include/gencec.h | 255 ++++++++++++++++++++ src/Makefile.am | 4 + src/gencec.c | 445 +++++++++++++++++++++++++++++++++++ 12 files changed, 1946 insertions(+) create mode 100644 AUTHORS create mode 100644 INSTALL create mode 100644 LICENSE create mode 100644 Makefile.am create mode 100644 README create mode 100644 configure.ac create mode 100644 doc/index.html create mode 100644 examples/Makefile.am create mode 100644 examples/cectest.c create mode 100644 include/gencec.h create mode 100644 m4/.gitkeep create mode 100644 src/Makefile.am create mode 100644 src/gencec.c
diff --git a/AUTHORS b/AUTHORS new file mode 100644 index 0000000..e4b7117 --- /dev/null +++ b/AUTHORS @@ -0,0 +1 @@ +Kamil Debski k.debski@samsung.com diff --git a/INSTALL b/INSTALL new file mode 100644 index 0000000..aac6101 --- /dev/null +++ b/INSTALL @@ -0,0 +1,9 @@ +To install libgencec run following commands:
+autoreconf -i
You might want add --force in here, otherwise the files will stay as-is if you update libtool and friends "mid-flight". Many projects include autogen.sh like the following. Feel free to add it to libgencec.
Thanks, I'll include this in the next version.
$cat autogen.sh #! /bin/sh
srcdir=`dirname "$0"` test -z "$srcdir" && srcdir=.
ORIGDIR=`pwd` cd "$srcdir"
autoreconf --force --verbose --install || exit 1 cd "$ORIGDIR" || exit $?
if test -z "$NOCONFIGURE"; then "$srcdir"/configure "$@" fi
--- /dev/null +++ b/configure.ac @@ -0,0 +1,24 @@
You can save yourself some headaches if you restrict old and/or buggy autoconf versions which don't work with the project. If I have to guess 2.60 should be ok. AC_PREREQ([XXX])
Good suggestion, thanks.
+AC_INIT([libgencec], [0.1], [k.debski@samsung.com]) +AM_INIT_AUTOMAKE([-Wall -Werror foreign])
+AC_PROG_CC +AM_PROG_AR +AC_CONFIG_MACRO_DIR([m4]) +AC_DEFINE([_GNU_SOURCE], [], [Use GNU extensions])
Same for libtool - 2.2 perhaps ? LT_PREREQ([XXX])
I agree.
+LT_INIT
+# Checks for typedefs, structures, and compiler characteristics. +AC_C_INLINE +AC_TYPE_SIZE_T +AC_TYPE_UINT16_T +AC_TYPE_UINT32_T +AC_TYPE_UINT8_T
+#AC_CHECK_LIB([c], [malloc]) +# Checks for library functions. +#AC_FUNC_MALLOC +l +AC_CONFIG_FILES([Makefile src/Makefile examples/Makefile])
Would be nice if the library provides libgencec.pc file. This way any users can avoid the explicit header/library check, amongst other useful bits.
Thanks for the suggestion, I'll look into this.
--- /dev/null +++ b/examples/Makefile.am @@ -0,0 +1,4 @@ +bin_PROGRAMS = cectest +cectest_SOURCES = cectest.c +AM_CPPFLAGS=-I$(top_srcdir)/include/ +AM_LDFLAGS=-L../src/ -lgencec
The following seems more common (in the projects I've seen at least) cectest_LDADD = $(top_builddir)/src/libgencec.la
diff --git a/m4/.gitkeep b/m4/.gitkeep new file mode 100644 index 0000000..e69de29
Haven't seen any projects doing this. Curious what the benefits of keeping and empty folder might be ?
When I run autoreconf -i it complained about missing m4 folder, hence I added this filler file such that the folder is created. Any suggestion on how to do this better?
diff --git a/src/Makefile.am b/src/Makefile.am new file mode 100644 index 0000000..cb024f0 --- /dev/null +++ b/src/Makefile.am @@ -0,0 +1,4 @@ +lib_LTLIBRARIES = libgencec.la +libgencec_la_SOURCES = gencec.c +libgencec_la_LDFLAGS = -version-info 0:1:0
You might want to add -no-undefined in order to prevent having a library with unresolved symbols.
Hope you find the above useful :-)
Thank you so much for your review. It is my first real approach at autotools, so your comments are very much appreciated :)
Cheers Emil --
Best wishes,