diff mbox

lcdapi: new package

Message ID 1350482898-5980-1-git-send-email-spdawson@gmail.com
State Superseded
Headers show

Commit Message

Simon Dawson Oct. 17, 2012, 2:08 p.m. UTC 
From: Simon Dawson <spdawson@gmail.com>

Signed-off-by: Simon Dawson <spdawson@gmail.com>
---
 package/Config.in                                  |    1 +
 package/lcdapi/Config.in                           |   11 +++
 package/lcdapi/lcdapi-fix-Makefile.patch           |   25 +++++++
 .../lcdapi-fix-missing-cstdlib-includes.patch      |   22 ++++++
 .../lcdapi-fix-missing-cstring-includes.patch      |   15 ++++
 package/lcdapi/lcdapi-make-toString-static.patch   |   19 +++++
 package/lcdapi/lcdapi.mk                           |   73 ++++++++++++++++++++
 7 files changed, 166 insertions(+)
 create mode 100644 package/lcdapi/Config.in
 create mode 100644 package/lcdapi/lcdapi-fix-Makefile.patch
 create mode 100644 package/lcdapi/lcdapi-fix-missing-cstdlib-includes.patch
 create mode 100644 package/lcdapi/lcdapi-fix-missing-cstring-includes.patch
 create mode 100644 package/lcdapi/lcdapi-make-toString-static.patch
 create mode 100644 package/lcdapi/lcdapi.mk

Comments

Arnout Vandecappelle Oct. 17, 2012, 8:12 p.m. UTC | #1 
On 17/10/12 16:08, spdawson@gmail.com wrote:
> From: Simon Dawson<spdawson@gmail.com>
>
> Signed-off-by: Simon Dawson<spdawson@gmail.com>
[snip]
> diff --git a/package/lcdapi/lcdapi-make-toString-static.patch b/package/lcdapi/lcdapi-make-toString-static.patch
> new file mode 100644
> index 0000000..0ef95fc
> --- /dev/null
> +++ b/package/lcdapi/lcdapi-make-toString-static.patch
> @@ -0,0 +1,19 @@
> +make the toString function defined in the LCDCallback.h header into a static
> +function, to fix link-time errors like the following.
> +
> +  multiple definition of `toString(char)'
> +
> +Signed-off-by: Simon Dawson<spdawson@gmail.com>
> +
> +diff -Nurp a/keys/LCDCallback.h b/keys/LCDCallback.h
> +--- a/keys/LCDCallback.h	2004-07-27 19:06:27.000000000 +0100
> ++++ b/keys/LCDCallback.h	2012-10-01 10:59:52.685708010 +0100
> +@@ -77,7 +77,7 @@ class LCDCallback
> +
> + typedef std::map<KeyEvent, LCDCallback *>  CallbackMap;
> +
> +-std::string toString(KeyEvent t)
> ++static std::string toString(KeyEvent t)

  Making it inline sounds more appropriate.

  BTW, did you upstream the patches?

> + {
> +   std::string s(1, (char)t);
> +   return s;
> diff --git a/package/lcdapi/lcdapi.mk b/package/lcdapi/lcdapi.mk
> new file mode 100644
> index 0000000..37e276c
> --- /dev/null
> +++ b/package/lcdapi/lcdapi.mk
> @@ -0,0 +1,73 @@
> +#############################################################
> +#
> +# lcdapi
> +#
> +#############################################################
> +LCDAPI_VERSION = 0.2
> +LCDAPI_SITE = ftp://ftp2.c-sait.net/csait
> +LCDAPI_DEPENDENCIES = host-lcdapi

  Why is this needed?  Or rather: I can build without this.
If it is not needed, the whole host-lcdapi can be removed BTW.

> +LCDAPI_INSTALL_STAGING = YES
> +
> +LCDAPI_LICENSE = LGPLv2.1+
> +LCDAPI_LICENSE_FILES = COPYING
> +
> +LCDAPI_MAKE_OPT = \
> +	$(TARGET_CONFIGURE_OPTS) \
> +	LD="$(TARGET_CXX)" \
> +	LDFLAGS="$(TARGET_LDFLAGS) -shared"

  It would make sense to also add CC="$(TARGET_CXX)" (although not strictly
necessary, because gcc recognizes the .cpp extension).

  More importantly, the CFLAGS overrides the CFLAGS of lcdapi's
Makefile, and that contains the all-important -fPIC.  I wonder
how you got it built without that...  So either:

- Set CFLAGS to "$(TARGET_CFLAGS) -fPIC"
- Patch the Makefile to append to CFLAGS
- Set CC to "$(TARGET_CXX) $(TARGET_CFLAGS)" and leave CFLAGS alone

  Finally, it would make sense to call it LCDAPI_MAKE_OPTS,
similar to TARGET_CONFIGURE_OPTS.

> +
> +HOST_LCDAPI_MAKE_OPT = \
> +	$(HOST_CONFIGURE_OPTS) \
> +	LD="$(HOSTCXX)" \
> +	LDFLAGS="$(HOST_LDFLAGS) -shared"
> +
> +define LCDAPI_BUILD_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(LCDAPI_MAKE_OPT) -C $(@D)
> +endef
> +
> +define HOST_LCDAPI_BUILD_CMDS
> +	$(HOST_MAKE_ENV) $(MAKE) $(HOST_LCDAPI_MAKE_OPT) -C $(@D)
> +endef
> +
> +define DO_LCDAPI_INSTALL
> +	$(INSTALL) -m 0755 -d $(1)/usr/lib
> +	$(INSTALL) -m 0755 -t $(1)/usr/lib $(@D)/lib/liblcdapi.so

  We would typically use
	$(INSTALL) -m 0755 -D $(@D)/lib/liblcdapi.so $(1)/usr/lib
(which does the mkdir and the install in one shot).

> +	$(INSTALL) -m 0755 -d $(1)/usr/include/lcdapi
> +	for i in api include keys sensors; do \
> +		$(INSTALL) -m 0755 -d $(1)/usr/include/lcdapi/$$i&&  \
> +		$(INSTALL) -m 0644 -t $(1)/usr/include/lcdapi/$$i \
> +			$(@D)/$$i/*.h; \

  Same here, but then iterating over api/*.h include/*.h keys/*.h sensors/*.h


  But wouldn't it be cleaner to add (and upstream) an 'install' target
to the Makefile?


  Regards,
  Arnout

> +	done
> +endef
> +
> +define DO_LCDAPI_UNINSTALL
> +	$(RM) $(1)/usr/lib/liblcdapi.so
> +	$(RM) -r $(1)/usr/include/lcdapi
> +endef
> +
> +define HOST_LCDAPI_INSTALL_CMDS
> +	$(call DO_LCDAPI_INSTALL,$(HOST_DIR))
> +endef
> +
> +define LCDAPI_INSTALL_STAGING_CMDS
> +	$(call DO_LCDAPI_INSTALL,$(STAGING_DIR))
> +endef
> +
> +define LCDAPI_INSTALL_TARGET_CMDS
> +	$(call DO_LCDAPI_INSTALL,$(TARGET_DIR))
> +endef
> +
> +define LCDAPI_UNINSTALL_STAGING_CMDS
> +	$(call DO_LCDAPI_UNINSTALL,$(STAGING_DIR))
> +endef
> +
> +define LCDAPI_UNINSTALL_TARGET_CMDS
> +	$(call DO_LCDAPI_UNINSTALL,$(TARGET_DIR))
> +endef
> +
> +define LCDAPI_CLEAN_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(LCDAPI_MAKE_OPT) -C $(@D) clean
> +endef
> +
> +$(eval $(generic-package))
> +$(eval $(host-generic-package))
Simon Dawson Oct. 18, 2012, 9:39 a.m. UTC | #2 
Hi Arnout. Thanks for taking the time to review this.

On 17 October 2012 21:12, Arnout Vandecappelle <arnout@mind.be> wrote:
>  Making it inline sounds more appropriate.

Okay; will do so.

>  BTW, did you upstream the patches?

Unfortunately, the upstream project is no longer being actively
maintained; I have attempted to contact the author, but have had no
response.

I have actually started work on a fork of the project at
https://github.com/spdawson/lcdapi; but I thought it best to use the
original source for the Buildroot package, at least in the first
instance.

>> +LCDAPI_DEPENDENCIES = host-lcdapi
>
>
>  Why is this needed?  Or rather: I can build without this.
> If it is not needed, the whole host-lcdapi can be removed BTW.

You're quite right: the host dependency is not required. This was
committed in error; I was using the dependency to trigger a host build
of the package during testing. I will remove the dependency.

>> +LCDAPI_MAKE_OPT = \
>> +       $(TARGET_CONFIGURE_OPTS) \
>> +       LD="$(TARGET_CXX)" \
>> +       LDFLAGS="$(TARGET_LDFLAGS) -shared"
>
>
>  It would make sense to also add CC="$(TARGET_CXX)" (although not strictly
> necessary, because gcc recognizes the .cpp extension).

Okay; will do.

>  More importantly, the CFLAGS overrides the CFLAGS of lcdapi's
> Makefile, and that contains the all-important -fPIC.  I wonder
> how you got it built without that...  So either:
>
> - Set CFLAGS to "$(TARGET_CFLAGS) -fPIC"
> - Patch the Makefile to append to CFLAGS
> - Set CC to "$(TARGET_CXX) $(TARGET_CFLAGS)" and leave CFLAGS alone

Okay, will do.

>  Finally, it would make sense to call it LCDAPI_MAKE_OPTS,
> similar to TARGET_CONFIGURE_OPTS.

Okay, will do.

>  We would typically use
>         $(INSTALL) -m 0755 -D $(@D)/lib/liblcdapi.so $(1)/usr/lib
> (which does the mkdir and the install in one shot).

Okay, will do.

>  But wouldn't it be cleaner to add (and upstream) an 'install' target
> to the Makefile?

Yes, okay.

Simon.
Arnout Vandecappelle Oct. 19, 2012, 8:04 p.m. UTC | #3 
On 18/10/12 11:39, Simon Dawson wrote:
> Hi Arnout. Thanks for taking the time to review this.
>
> On 17 October 2012 21:12, Arnout Vandecappelle<arnout@mind.be>  wrote:
>>   Making it inline sounds more appropriate.
>
> Okay; will do so.
>
>>   BTW, did you upstream the patches?
>
> Unfortunately, the upstream project is no longer being actively
> maintained; I have attempted to contact the author, but have had no
> response.
>
> I have actually started work on a fork of the project at
> https://github.com/spdawson/lcdapi; but I thought it best to use the
> original source for the Buildroot package, at least in the first
> instance.

  I don't think that's necessary.  It's better if we don't have to
carry patches in buildroot!

[snip]
>>> +LCDAPI_MAKE_OPT = \
>>> +       $(TARGET_CONFIGURE_OPTS) \
>>> +       LD="$(TARGET_CXX)" \
>>> +       LDFLAGS="$(TARGET_LDFLAGS) -shared"
>>
>>
>>   It would make sense to also add CC="$(TARGET_CXX)" (although not strictly
>> necessary, because gcc recognizes the .cpp extension).
>
> Okay; will do.

  If you're anyway taking over upstream, you could really fix the Makefiles
and use CXX instead of CC!


>>   More importantly, the CFLAGS overrides the CFLAGS of lcdapi's
>> Makefile, and that contains the all-important -fPIC.  I wonder
>> how you got it built without that...  So either:
>>
>> - Set CFLAGS to "$(TARGET_CFLAGS) -fPIC"
>> - Patch the Makefile to append to CFLAGS
>> - Set CC to "$(TARGET_CXX) $(TARGET_CFLAGS)" and leave CFLAGS alone
>
> Okay, will do.

  Again, best to patch the Makefile since you can.


>>   Finally, it would make sense to call it LCDAPI_MAKE_OPTS,
>> similar to TARGET_CONFIGURE_OPTS.
>
> Okay, will do.
>
>>   We would typically use
>>          $(INSTALL) -m 0755 -D $(@D)/lib/liblcdapi.so $(1)/usr/lib
>> (which does the mkdir and the install in one shot).
>
> Okay, will do.
>
>>   But wouldn't it be cleaner to add (and upstream) an 'install' target
>> to the Makefile?
>
> Yes, okay.

  Sorry that I'm giving you more work :-)

  Regards,
  Arnout
Simon Dawson Oct. 24, 2012, 9:07 a.m. UTC | #4 
On 19 October 2012 21:04, Arnout Vandecappelle <arnout@mind.be> wrote:
>  Sorry that I'm giving you more work :-)

Hi Arnout. No worries --- thanks for the comments and suggestions.
I'll rework the patch to use the new upstream project, which should
make things a lot cleaner.

Simon.
diff mbox

Patch

diff --git a/package/Config.in b/package/Config.in
index d7b3db6..8e2f188 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -385,6 +385,7 @@  endmenu
 
 menu "Hardware handling"
 source "package/ccid/Config.in"
+source "package/lcdapi/Config.in"
 source "package/libaio/Config.in"
 source "package/libraw1394/Config.in"
 source "package/tslib/Config.in"
diff --git a/package/lcdapi/Config.in b/package/lcdapi/Config.in
new file mode 100644
index 0000000..cb97a49
--- /dev/null
+++ b/package/lcdapi/Config.in
@@ -0,0 +1,11 @@ 
+config BR2_PACKAGE_LCDAPI
+	bool "lcdapi"
+	depends on BR2_INSTALL_LIBSTDCPP
+	depends on BR2_TOOLCHAIN_HAS_THREADS
+	help
+	  C++ client API for lcdproc, containing a set of widget classes.
+
+	  http://www.c-sait.net/lcdapi/
+
+comment "lcdapi requires a toolchain with C++ and thread support enabled"
+	depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS
diff --git a/package/lcdapi/lcdapi-fix-Makefile.patch b/package/lcdapi/lcdapi-fix-Makefile.patch
new file mode 100644
index 0000000..1c619d5
--- /dev/null
+++ b/package/lcdapi/lcdapi-fix-Makefile.patch
@@ -0,0 +1,25 @@ 
+Fix a presumed typo in the Makfile: $(INC_FLAGS) should be $(INCFLAGS), it
+would appear.
+
+Also link against the stdc++ and pthread libraries explicitly.
+
+Signed-off-by: Simon Dawson <spdawson@gmail.com>
+
+diff -Nurp a/Makefile b/Makefile
+--- a/Makefile	2004-07-27 19:06:27.000000000 +0100
++++ b/Makefile	2012-10-01 09:15:52.385986229 +0100
+@@ -64,12 +64,12 @@ deliver: clean doc_clean
+ 
+ $(LIB_TARGET): $(LIB_OBJS)
+ 	@$(MKDIR) $(LIB_DIR)
+-	$(LD) $(LDFLAGS) -o $(LIB_TARGET) $(LIB_OBJS)
++	$(LD) $(LDFLAGS) -o $(LIB_TARGET) $(LIB_OBJS) -lstdc++ -lpthread
+ 
+ 
+ $(OBJ_DIR)/%.o: %.cpp
+ 	@$(MKDIR) $(OBJ_DIR)/api $(OBJ_DIR)/sensors $(OBJ_DIR)/keys
+-	$(CC) $(CFLAGS) $(INC_FLAGS) -c -o $@ $<
++	$(CC) $(CFLAGS) $(INCFLAGS) -c -o $@ $<
+ 
+ 
+ # DO NOT DELETE
diff --git a/package/lcdapi/lcdapi-fix-missing-cstdlib-includes.patch b/package/lcdapi/lcdapi-fix-missing-cstdlib-includes.patch
new file mode 100644
index 0000000..c47aee0
--- /dev/null
+++ b/package/lcdapi/lcdapi-fix-missing-cstdlib-includes.patch
@@ -0,0 +1,22 @@ 
+Add some missing include directives for the cstdlib header.
+
+Signed-off-by: Simon Dawson <spdawson@gmail.com>
+
+diff -Nurp a/api/LCDBar.cpp b/api/LCDBar.cpp
+--- a/api/LCDBar.cpp	2004-07-27 19:06:27.000000000 +0100
++++ b/api/LCDBar.cpp	2012-10-01 08:18:37.570139543 +0100
+@@ -1,4 +1,5 @@
+ #include "LCDBar.h"
++#include <cstdlib>
+ #include <sstream>
+ #include <stdlib.h>
+ 
+diff -Nurp a/api/LCDBigNumber.cpp b/api/LCDBigNumber.cpp
+--- a/api/LCDBigNumber.cpp	2004-07-27 19:06:27.000000000 +0100
++++ b/api/LCDBigNumber.cpp	2012-10-01 08:18:13.066140468 +0100
+@@ -1,4 +1,5 @@
+ #include "LCDBigNumber.h"
++#include <cstdlib>
+ #include <sstream>
+ #include <string>
+ 
diff --git a/package/lcdapi/lcdapi-fix-missing-cstring-includes.patch b/package/lcdapi/lcdapi-fix-missing-cstring-includes.patch
new file mode 100644
index 0000000..8d5dffc
--- /dev/null
+++ b/package/lcdapi/lcdapi-fix-missing-cstring-includes.patch
@@ -0,0 +1,15 @@ 
+Add some missing include directives for the cstring header.
+
+Signed-off-by: Simon Dawson <spdawson@gmail.com>
+
+diff -Nurp a/sensors/LCDSensor.cpp b/sensors/LCDSensor.cpp
+--- a/sensors/LCDSensor.cpp	2004-07-27 19:06:27.000000000 +0100
++++ b/sensors/LCDSensor.cpp	2012-10-01 08:43:13.726073554 +0100
+@@ -2,6 +2,7 @@
+ #include "LCDUtils.h"
+ #include "LCDSensor.h"
+ 
++#include <cstring>
+ #include <string>
+ #include <stdio.h>
+ #include <unistd.h>
diff --git a/package/lcdapi/lcdapi-make-toString-static.patch b/package/lcdapi/lcdapi-make-toString-static.patch
new file mode 100644
index 0000000..0ef95fc
--- /dev/null
+++ b/package/lcdapi/lcdapi-make-toString-static.patch
@@ -0,0 +1,19 @@ 
+make the toString function defined in the LCDCallback.h header into a static
+function, to fix link-time errors like the following.
+
+  multiple definition of `toString(char)'
+
+Signed-off-by: Simon Dawson <spdawson@gmail.com>
+
+diff -Nurp a/keys/LCDCallback.h b/keys/LCDCallback.h
+--- a/keys/LCDCallback.h	2004-07-27 19:06:27.000000000 +0100
++++ b/keys/LCDCallback.h	2012-10-01 10:59:52.685708010 +0100
+@@ -77,7 +77,7 @@ class LCDCallback
+ 
+ typedef std::map<KeyEvent, LCDCallback *> CallbackMap;
+ 
+-std::string toString(KeyEvent t)
++static std::string toString(KeyEvent t)
+ {
+   std::string s(1, (char)t);
+   return s;
diff --git a/package/lcdapi/lcdapi.mk b/package/lcdapi/lcdapi.mk
new file mode 100644
index 0000000..37e276c
--- /dev/null
+++ b/package/lcdapi/lcdapi.mk
@@ -0,0 +1,73 @@ 
+#############################################################
+#
+# lcdapi
+#
+#############################################################
+LCDAPI_VERSION = 0.2
+LCDAPI_SITE = ftp://ftp2.c-sait.net/csait
+LCDAPI_DEPENDENCIES = host-lcdapi
+LCDAPI_INSTALL_STAGING = YES
+
+LCDAPI_LICENSE = LGPLv2.1+
+LCDAPI_LICENSE_FILES = COPYING
+
+LCDAPI_MAKE_OPT = \
+	$(TARGET_CONFIGURE_OPTS) \
+	LD="$(TARGET_CXX)" \
+	LDFLAGS="$(TARGET_LDFLAGS) -shared"
+
+HOST_LCDAPI_MAKE_OPT = \
+	$(HOST_CONFIGURE_OPTS) \
+	LD="$(HOSTCXX)" \
+	LDFLAGS="$(HOST_LDFLAGS) -shared"
+
+define LCDAPI_BUILD_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(LCDAPI_MAKE_OPT) -C $(@D)
+endef
+
+define HOST_LCDAPI_BUILD_CMDS
+	$(HOST_MAKE_ENV) $(MAKE) $(HOST_LCDAPI_MAKE_OPT) -C $(@D)
+endef
+
+define DO_LCDAPI_INSTALL
+	$(INSTALL) -m 0755 -d $(1)/usr/lib
+	$(INSTALL) -m 0755 -t $(1)/usr/lib $(@D)/lib/liblcdapi.so
+	$(INSTALL) -m 0755 -d $(1)/usr/include/lcdapi
+	for i in api include keys sensors; do \
+		$(INSTALL) -m 0755 -d $(1)/usr/include/lcdapi/$$i && \
+		$(INSTALL) -m 0644 -t $(1)/usr/include/lcdapi/$$i \
+			$(@D)/$$i/*.h; \
+	done
+endef
+
+define DO_LCDAPI_UNINSTALL
+	$(RM) $(1)/usr/lib/liblcdapi.so
+	$(RM) -r $(1)/usr/include/lcdapi
+endef
+
+define HOST_LCDAPI_INSTALL_CMDS
+	$(call DO_LCDAPI_INSTALL,$(HOST_DIR))
+endef
+
+define LCDAPI_INSTALL_STAGING_CMDS
+	$(call DO_LCDAPI_INSTALL,$(STAGING_DIR))
+endef
+
+define LCDAPI_INSTALL_TARGET_CMDS
+	$(call DO_LCDAPI_INSTALL,$(TARGET_DIR))
+endef
+
+define LCDAPI_UNINSTALL_STAGING_CMDS
+	$(call DO_LCDAPI_UNINSTALL,$(STAGING_DIR))
+endef
+
+define LCDAPI_UNINSTALL_TARGET_CMDS
+	$(call DO_LCDAPI_UNINSTALL,$(TARGET_DIR))
+endef
+
+define LCDAPI_CLEAN_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(LCDAPI_MAKE_OPT) -C $(@D) clean
+endef
+
+$(eval $(generic-package))
+$(eval $(host-generic-package))