diff mbox

[1/1] chdkptp CLI added

Message ID 1387798250-2437-2-git-send-email-matthew.j.urry@gmail.com
State Rejected
Headers show

Commit Message

Matthew Urry Dec. 23, 2013, 11:30 a.m. UTC 
Signed-off-by: Matthew Urry <matthew.j.urry@gmail.com>
---
 package/Config.in                                  |    1 +
 package/chdkptp/Config.in                          |   15 +++++++++++
 .../chdkptp-01-add_lm_flag_to_makefile.patch       |   15 +++++++++++
 ...ptp-02-create_sh_script_to_launch_chdkptp.patch |   22 ++++++++++++++++
 package/chdkptp/chdkptp.mk                         |   27 ++++++++++++++++++++
 5 files changed, 80 insertions(+)
 create mode 100644 package/chdkptp/Config.in
 create mode 100644 package/chdkptp/chdkptp-01-add_lm_flag_to_makefile.patch
 create mode 100644 package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch
 create mode 100644 package/chdkptp/chdkptp.mk

Comments

Thomas Petazzoni Dec. 28, 2013, 4:38 p.m. UTC | #1 
Dear Matthew Urry,

Thanks for this contribution. Note that for a single patch, there is no
real need to send a cover letter (i.e the mail title PATCH 0/1). The
text of your cover letter could almost as is have been part of the
commit log itself (with just "I have only enabled the command line
interface and not the GUI." changed to something less personal like
"The package only builds the command line interface and not the GUI").

Some more comments below.

On Mon, 23 Dec 2013 11:30:50 +0000, Matthew Urry wrote:

> diff --git a/package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch b/package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch
> new file mode 100644
> index 0000000..aadd6c7
> --- /dev/null
> +++ b/package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch

There is no description for this patch. And it seems to only be
renaming a file. Why is this needed? Why don't you do it in the
package .mk file instead?

> --- /dev/null
> +++ b/package/chdkptp/chdkptp.mk
> @@ -0,0 +1,27 @@
> +################################################################################
> +#
> +# chdkptp
> +#
> +################################################################################
> +
> +CHDKPTP_VERSION = 461
> +CHDKPTP_SITE = http://subversion.assembla.com/svn/chdkptp/trunk
> +CHDKPTP_SITE_METHOD = svn
> +CHDKPTP_DEPENDENCIES = libusb-compat lua

Is 'lua' really a build dependency? Or is it only a runtime dependency?

> +CHDKPTP_LICENSE = GPLv2

License seems to be GPLv2+.

> +define CHDKPTP_BUILD_CMDS
> +	$(MAKE) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -C $(@D) 

We generally like to also pass CFLAGS and LDFLAGS here by doing:

	$(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D)

though that might require changing a bit the project Makefile to use
constructs like:

override CFLAGS += ...
override LDFLAGS += ...

Note that doing that would also allow to avoid the -lm patch altogether.

> +endef
> +
> +define CHDKPTP_INSTALL_TARGET_CMDS
> +        $(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/share/chdkptp
> +        $(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/share/chdkptp/lua
> +        $(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/share/chdkptp/lua/extras

I believe that last one is sufficient: $(INSTALL) automatically creates
intermediate directories if needed.

> +    	$(INSTALL) -m 0755 $(@D)/chdkptp $(TARGET_DIR)/usr/share/chdkptp
> +        $(INSTALL) -m 0755 $(@D)/lua/*.lua $(TARGET_DIR)/usr/share/chdkptp/lua
> +        $(INSTALL) -m 0755 $(@D)/lua/extras/*.lua $(TARGET_DIR)/usr/share/chdkptp/lua/extras
> +        $(INSTALL) -m 0755 $(@D)/chdkptp.sh $(TARGET_DIR)/usr/bin/chdkptp

I haven't looked, but why is usr/bin/chdkptp a shell script, and the
real binary installed in /usr/share/chdkptp ? Adding a comment above
the install target commands macro explaining why it is needed would be
useful.

Could you take into account these comments and submit an updated
version? Thanks a lot!

Thomas
Thomas Petazzoni March 7, 2014, 10:50 p.m. UTC | #2 
Dear Matthew Urry,

On Mon, 23 Dec 2013 11:30:50 +0000, Matthew Urry wrote:
> 
> Signed-off-by: Matthew Urry <matthew.j.urry@gmail.com>
> ---
>  package/Config.in                                  |    1 +
>  package/chdkptp/Config.in                          |   15 +++++++++++
>  .../chdkptp-01-add_lm_flag_to_makefile.patch       |   15 +++++++++++
>  ...ptp-02-create_sh_script_to_launch_chdkptp.patch |   22 ++++++++++++++++
>  package/chdkptp/chdkptp.mk                         |   27 ++++++++++++++++++++
>  5 files changed, 80 insertions(+)
>  create mode 100644 package/chdkptp/Config.in
>  create mode 100644 package/chdkptp/chdkptp-01-add_lm_flag_to_makefile.patch
>  create mode 100644 package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch
>  create mode 100644 package/chdkptp/chdkptp.mk

You received some review on this patch on December, 28th 2013,
suggesting several improvements to make your patch acceptable upstream.
Your contributions are definitely welcome, but if you want to get it
accepted, it'd be nice if you could resend a new version that takes
into account the comments that have been made.

Thanks a lot!

Thomas
Thomas Petazzoni June 11, 2014, 8:28 p.m. UTC | #3 
Hello Matthew,

On Fri, 7 Mar 2014 23:50:55 +0100, Thomas Petazzoni wrote:

> On Mon, 23 Dec 2013 11:30:50 +0000, Matthew Urry wrote:
> > 
> > Signed-off-by: Matthew Urry <matthew.j.urry@gmail.com>
> > ---
> >  package/Config.in                                  |    1 +
> >  package/chdkptp/Config.in                          |   15 +++++++++++
> >  .../chdkptp-01-add_lm_flag_to_makefile.patch       |   15 +++++++++++
> >  ...ptp-02-create_sh_script_to_launch_chdkptp.patch |   22 ++++++++++++++++
> >  package/chdkptp/chdkptp.mk                         |   27 ++++++++++++++++++++
> >  5 files changed, 80 insertions(+)
> >  create mode 100644 package/chdkptp/Config.in
> >  create mode 100644 package/chdkptp/chdkptp-01-add_lm_flag_to_makefile.patch
> >  create mode 100644 package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch
> >  create mode 100644 package/chdkptp/chdkptp.mk
> 
> You received some review on this patch on December, 28th 2013,
> suggesting several improvements to make your patch acceptable upstream.
> Your contributions are definitely welcome, but if you want to get it
> accepted, it'd be nice if you could resend a new version that takes
> into account the comments that have been made.

Since you never came back to the Buildroot community with an updated
patch after about 6 months, despite us giving some review comments, we
will now mark the patch as Rejected.

Of course, if you're still interested in seeing this patch merged,
do not hesitate to send a new iteration that takes into account the
comments that were made.

See http://patchwork.ozlabs.org/patch/304744/ for the review discussion.

Thanks for your contribution!

Thomas
diff mbox

Patch

diff --git a/package/Config.in b/package/Config.in
index 3685807..369d361 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -259,6 +259,7 @@  endmenu
 source "package/a10disp/Config.in"
 source "package/acpid/Config.in"
 source "package/cdrkit/Config.in"
+source "package/chdkptp/Config.in"
 source "package/cryptsetup/Config.in"
 source "package/dbus/Config.in"
 source "package/dbus-glib/Config.in"
diff --git a/package/chdkptp/Config.in b/package/chdkptp/Config.in
new file mode 100644
index 0000000..cfd2de9
--- /dev/null
+++ b/package/chdkptp/Config.in
@@ -0,0 +1,15 @@ 
+config BR2_PACKAGE_CHDKPTP
+	bool "chdkptp"
+	select BR2_PACKAGE_LUA 
+	select BR2_PACKAGE_LIBUSB
+	select BR2_PACKAGE_LIBUSB_COMPAT
+	depends on BR2_LARGEFILE
+	depends on BR2_USE_WCHAR
+	help
+	  chdkptp is a client application for interacting with
+	  the chdk ptp extension. This will complie the CLI only.
+	  
+	  http://chdk.wikia.com/wiki/CHDK
+	  https://www.assembla.com/spaces/chdkptp/wiki
+comment "chdkptp needs a toolchain w/ largefile and wchar"
+	depends on (!BR2_LARGEFILE || !BR2_USE_WCHAR)
diff --git a/package/chdkptp/chdkptp-01-add_lm_flag_to_makefile.patch b/package/chdkptp/chdkptp-01-add_lm_flag_to_makefile.patch
new file mode 100644
index 0000000..82aeedd
--- /dev/null
+++ b/package/chdkptp/chdkptp-01-add_lm_flag_to_makefile.patch
@@ -0,0 +1,15 @@ 
+Makefile: add -lm to linker flags
+
+Signed-off-by: Matthew Urry <matthew.j.urry@gmail.com>
+
+diff -purN chdkptp-461.orig/Makefile chdkptp-461/Makefile
+--- chdkptp-461.orig/Makefile	2013-12-08 12:10:18.469855434 +0000
++++ chdkptp-461/Makefile	2013-12-08 12:11:16.164098345 +0000
+@@ -88,6 +88,7 @@ INC_PATHS+=-I$(CHDK_SRC_DIR)
+ CFLAGS+=$(INC_PATHS) -DCHDKPTP_BUILD_NUM=$(SVNREV) -DCHDKPTP_REL_DESC="\"alpha\""
+ 
+ LDFLAGS+=$(LIB_PATHS) $(patsubst %,-l%,$(LINK_LIBS) $(SYS_LIBS)) 
++LDFLAGS+=-lm
+ 
+ SUBDIRS=lfs
+ 
diff --git a/package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch b/package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch
new file mode 100644
index 0000000..aadd6c7
--- /dev/null
+++ b/package/chdkptp/chdkptp-02-create_sh_script_to_launch_chdkptp.patch
@@ -0,0 +1,22 @@ 
+diff -purN chdkptp/chdkptp-sample.sh chdkptp-patched/chdkptp-sample.sh
+--- chdkptp/chdkptp-sample.sh	2013-12-23 09:06:03.772637062 +0000
++++ chdkptp-patched/chdkptp-sample.sh	1970-01-01 01:00:00.000000000 +0100
+@@ -1,7 +0,0 @@
+-#!/bin/sh
+-# adjust the following to your configuration
+-CHDKPTP_DIR=/path/to/chdkptp
+-# only need if you have compiled IUP support and have NOT installed the libraries to system directories 
+-#export LD_LIBRARY_PATH=/path/to/iup:/path/to/cd
+-export LUA_PATH="$CHDKPTP_DIR/lua/?.lua"
+-"$CHDKPTP_DIR/chdkptp" "$@"
+diff -purN chdkptp/chdkptp.sh chdkptp-patched/chdkptp.sh
+--- chdkptp/chdkptp.sh	1970-01-01 01:00:00.000000000 +0100
++++ chdkptp-patched/chdkptp.sh	2013-12-23 09:08:51.868642764 +0000
+@@ -0,0 +1,7 @@
++#!/bin/sh
++# adjust the following to your configuration
++CHDKPTP_DIR=/usr/share/chdkptp
++# only need if you have compiled IUP support and have NOT installed the libraries to system directories 
++#export LD_LIBRARY_PATH=/path/to/iup:/path/to/cd
++export LUA_PATH="$CHDKPTP_DIR/lua/?.lua"
++"$CHDKPTP_DIR/chdkptp" "$@"
diff --git a/package/chdkptp/chdkptp.mk b/package/chdkptp/chdkptp.mk
new file mode 100644
index 0000000..80b6a60
--- /dev/null
+++ b/package/chdkptp/chdkptp.mk
@@ -0,0 +1,27 @@ 
+################################################################################
+#
+# chdkptp
+#
+################################################################################
+
+CHDKPTP_VERSION = 461
+CHDKPTP_SITE = http://subversion.assembla.com/svn/chdkptp/trunk
+CHDKPTP_SITE_METHOD = svn
+CHDKPTP_DEPENDENCIES = libusb-compat lua
+CHDKPTP_LICENSE = GPLv2
+
+define CHDKPTP_BUILD_CMDS
+	$(MAKE) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -C $(@D) 
+endef
+
+define CHDKPTP_INSTALL_TARGET_CMDS
+        $(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/share/chdkptp
+        $(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/share/chdkptp/lua
+        $(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/share/chdkptp/lua/extras
+    	$(INSTALL) -m 0755 $(@D)/chdkptp $(TARGET_DIR)/usr/share/chdkptp
+        $(INSTALL) -m 0755 $(@D)/lua/*.lua $(TARGET_DIR)/usr/share/chdkptp/lua
+        $(INSTALL) -m 0755 $(@D)/lua/extras/*.lua $(TARGET_DIR)/usr/share/chdkptp/lua/extras
+        $(INSTALL) -m 0755 $(@D)/chdkptp.sh $(TARGET_DIR)/usr/bin/chdkptp
+endef
+
+$(eval $(generic-package))