Message ID | 1350482898-5980-1-git-send-email-spdawson@gmail.com |
---|---|
State | Superseded |
Headers | show |
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))
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.
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
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 --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))