[Buildroot] [PATCH 1/2] Cdrtools is a set of command line programs that allows to record CD/DVD/BluRay media. This package contains the cdrecord & mkisofs programs.

[Buildroot] [PATCH 1/2] Cdrtools is a set of command line programs that allows to record CD/DVD/BluRay media. This package contains the cdrecord & mkisofs programs.

[Steve Kenton]

http://sourceforge.net/projects/cdrtools
Signed-off-by: Steve Kenton <skenton@ou.edu>
---
 package/Config.in            |  1 +
 package/cdrtools/Config.in   | 12 ++++++++++++
 package/cdrtools/cdrtools.mk | 21 +++++++++++++++++++++
 3 files changed, 34 insertions(+)
 create mode 100644 package/cdrtools/Config.in
 create mode 100644 package/cdrtools/cdrtools.mk

diff --git a/package/Config.in b/package/Config.in
index dd011be..1748e88 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -6,6 +6,7 @@ menu "Audio and video applications"
 	source "package/alsa-utils/Config.in"
 	source "package/aumix/Config.in"
 	source "package/bellagio/Config.in"
+	source "package/cdrtools/Config.in"
 	source "package/dvdauthor/Config.in"
 	source "package/espeak/Config.in"
 	source "package/faad2/Config.in"
diff --git a/package/cdrtools/Config.in b/package/cdrtools/Config.in
new file mode 100644
index 0000000..8583cec
--- /dev/null
+++ b/package/cdrtools/Config.in
@@ -0,0 +1,12 @@
+config BR2_PACKAGE_CDRTOOLS
+	bool "cdrtools"
+	depends on BR2_USE_WCHAR && BR2_LARGEFILE
+	help
+	  Cdrtools is a set of command line programs that
+	  allows to record CD/DVD/BluRay media. This
+	  package contains the cdrecord & mkisofs programs.
+	  
+	  http://sourceforge.net/projects/cdrtools
+
+comment "cdrtools needs a toolchain w/ wchar, largefile"
+	depends on !BR2_USE_WCHAR || !BR2_LARGEFILE
diff --git a/package/cdrtools/cdrtools.mk b/package/cdrtools/cdrtools.mk
new file mode 100644
index 0000000..a732e69
--- /dev/null
+++ b/package/cdrtools/cdrtools.mk
@@ -0,0 +1,21 @@
+#############################################################
+#
+# cdrtools
+#
+#############################################################
+CDRTOOLS_VERSION = 3.00
+CDRTOOLS_SITE = http://sourceforge.net/projects/cdrtools/files
+CDRTOOLS_LICENSE = Various parts under GPLv2.0, LGPLv2.1, CDDL - see COPYING
+CDRTOOLS_LICENSE_FILES = COPYING
+
+define CDRTOOLS_BUILD_CMDS
+	$(MAKE) -C $(@D)
+endef
+
+define CDRTOOLS_INSTALL_TARGET_CMDS
+	$(INSTALL) -m 0755 -D $(@D)/cdrecord/OBJ/x86_64-linux-cc/cdrecord $(TARGET_DIR)/usr/bin/cdrecord
+	$(INSTALL) -m 0755 -D $(@D)/readcd/OBJ/x86_64-linux-cc/readcd $(TARGET_DIR)/usr/bin/readcd
+	$(INSTALL) -m 0755 -D $(@D)/mkisofs/OBJ/x86_64-linux-cc/mkisofs $(TARGET_DIR)/usr/bin/mkisofs
+endef
+
+$(eval $(generic-package))
-- 
1.9.1

[Buildroot] [PATCH 2/2] Add hash file

[Steve Kenton]

Signed-off-by: Steve Kenton <skenton@ou.edu>
---
 package/cdrtools/cdrtools.hash | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 package/cdrtools/cdrtools.hash

diff --git a/package/cdrtools/cdrtools.hash b/package/cdrtools/cdrtools.hash
new file mode 100644
index 0000000..c9f9c29
--- /dev/null
+++ b/package/cdrtools/cdrtools.hash
@@ -0,0 +1,3 @@
+cdrtools-3.00.tar.gz 	2010-06-02 	2.5 MB 	6363 weekly downloads 	i
+md5	bb21cefefcfbb76cf249120e8978ffdd	cdrtools-3.00.tar.gz 
+sha1	2cd7d1725e0da2267b7a033cc744295d6e2bc6b9	cdrtools-3.00.tar.gz 
-- 
1.9.1

[Buildroot] [PATCH 2/2] Add hash file

[Thomas Petazzoni]

Dear Steve Kenton,

On Mon,  2 Feb 2015 05:40:53 +0000, Steve Kenton wrote:
> Signed-off-by: Steve Kenton <skenton@ou.edu>
> ---
>  package/cdrtools/cdrtools.hash | 3 +++
>  1 file changed, 3 insertions(+)
>  create mode 100644 package/cdrtools/cdrtools.hash

This patch should not be separated from the previous, it should be
merged with it.

> 
> diff --git a/package/cdrtools/cdrtools.hash b/package/cdrtools/cdrtools.hash
> new file mode 100644
> index 0000000..c9f9c29
> --- /dev/null
> +++ b/package/cdrtools/cdrtools.hash
> @@ -0,0 +1,3 @@
> +cdrtools-3.00.tar.gz 	2010-06-02 	2.5 MB 	6363 weekly downloads 	i

This line doesn't make sense in a hash file. There should instead be a
comment saying where the hashes are coming from. Something like:

# From http://sourceforge.net/....

or

# Locally computed

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 2/2] Add hash file

[Steve Kenton]

On 2/2/2015 3:50 AM, Thomas Petazzoni wrote:
> Dear Steve Kenton,
>
> On Mon,  2 Feb 2015 05:40:53 +0000, Steve Kenton wrote:
>> Signed-off-by: Steve Kenton<skenton@ou.edu>
>> ---
>>   package/cdrtools/cdrtools.hash | 3 +++
>>   1 file changed, 3 insertions(+)
>>   create mode 100644 package/cdrtools/cdrtools.hash
> This patch should not be separated from the previous, it should be
> merged with it.
This was my first attempt at git. I tried to follow the directions in 
the buildroot manual but
since these were separate commits they generated separate emails using the
git format patch command example. What should it be to make it all one 
patch?

Steve

[Buildroot] [PATCH 2/2] Add hash file

[Thomas Petazzoni]

Dear Steve Kenton,

On Mon, 02 Feb 2015 09:03:19 -0600, Steve Kenton wrote:

> This was my first attempt at git. I tried to follow the directions in 
> the buildroot manual but
> since these were separate commits they generated separate emails using the
> git format patch command example. What should it be to make it all one 
> patch?

Look at "Interactive git rebasing", using 'git rebase -i'. It allows
you to reorder patches, merge patches, edit patches, etc.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/2] Cdrtools is a set of command line programs that allows to record CD/DVD/BluRay media. This package contains the cdrecord & mkisofs programs.

[Thomas Petazzoni]

Dear Steve Kenton,

Have you looked at the 'cdrkit' package? I believe it bundles the same
tools as cdrtools, but with a sane build system and not this
brain-damaged build system that cdrtools uses.

That being said, here is a review of your patch below.

First, the title should be simply:

	cdrtools: new package

On Mon,  2 Feb 2015 05:40:52 +0000, Steve Kenton wrote:

> diff --git a/package/Config.in b/package/Config.in
> index dd011be..1748e88 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -6,6 +6,7 @@ menu "Audio and video applications"
>  	source "package/alsa-utils/Config.in"
>  	source "package/aumix/Config.in"
>  	source "package/bellagio/Config.in"
> +	source "package/cdrtools/Config.in"

Since it's really the same sort of tools than cdrkit, I'd rather see
cdrtools next to cdrkit in the menus.

> diff --git a/package/cdrtools/Config.in b/package/cdrtools/Config.in
> new file mode 100644
> index 0000000..8583cec
> --- /dev/null
> +++ b/package/cdrtools/Config.in
> @@ -0,0 +1,12 @@
> +config BR2_PACKAGE_CDRTOOLS
> +	bool "cdrtools"
> +	depends on BR2_USE_WCHAR && BR2_LARGEFILE

We generally put this on two separate lines.

> +	help
> +	  Cdrtools is a set of command line programs that
> +	  allows to record CD/DVD/BluRay media. This
> +	  package contains the cdrecord & mkisofs programs.
> +	  
> +	  http://sourceforge.net/projects/cdrtools
> +
> +comment "cdrtools needs a toolchain w/ wchar, largefile"
> +	depends on !BR2_USE_WCHAR || !BR2_LARGEFILE
> diff --git a/package/cdrtools/cdrtools.mk b/package/cdrtools/cdrtools.mk
> new file mode 100644
> index 0000000..a732e69
> --- /dev/null
> +++ b/package/cdrtools/cdrtools.mk
> @@ -0,0 +1,21 @@
> +#############################################################
> +#
> +# cdrtools
> +#
> +#############################################################

You should have 80 # signs for the header. And one empty new line
between the header and the first variable.

> +CDRTOOLS_VERSION = 3.00
> +CDRTOOLS_SITE = http://sourceforge.net/projects/cdrtools/files
> +CDRTOOLS_LICENSE = Various parts under GPLv2.0, LGPLv2.1, CDDL - see COPYING

Don't use "see COPYING", this is already encoded by the
<pkg>_LICENSE_FILES variable below.

> +CDRTOOLS_LICENSE_FILES = COPYING
> +
> +define CDRTOOLS_BUILD_CMDS
> +	$(MAKE) -C $(@D)
> +endef

This unfortunately doesn't work at all: it builds the tools with the
host compiler, so always for x86 or x86-64. Is this your intention? If
it is your intention, then it should be a host package, and not a
target package.

> +
> +define CDRTOOLS_INSTALL_TARGET_CMDS
> +	$(INSTALL) -m 0755 -D $(@D)/cdrecord/OBJ/x86_64-linux-cc/cdrecord $(TARGET_DIR)/usr/bin/cdrecord
> +	$(INSTALL) -m 0755 -D $(@D)/readcd/OBJ/x86_64-linux-cc/readcd $(TARGET_DIR)/usr/bin/readcd
> +	$(INSTALL) -m 0755 -D $(@D)/mkisofs/OBJ/x86_64-linux-cc/mkisofs $(TARGET_DIR)/usr/bin/mkisofs
> +endef

This cannot work: you enforce "x86_64-linux-cc", but we might be
building for ARM, PowerPC, MIPS, etc. Also, isn't there a "make
install" rule to simplify the installation logic?

So basically, try to do a build for ARM, and make sure 1/ it builds
fine and 2/ the installed binaries are actually built for ARM.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/2] Cdrtools is a set of command line programs that allows to record CD/DVD/BluRay media. This package contains the cdrecord & mkisofs programs.

[Steve Kenton]

On 2/2/2015 3:35 AM, Thomas Petazzoni wrote:
> Dear Steve Kenton,
>
> Have you looked at the 'cdrkit' package? I believe it bundles the same
> tools as cdrtools, but with a sane build system and not this
> brain-damaged build system that cdrtools uses
Yes, I started with cdrkit, but genisofs and wodim did not do everything 
we needed.
We're needing UDF support and while I like xorriso it also does not do 
everything we need.
I don't remember right off what all went wrong but it was with some 
reluctance that I returned to cdrtools
for the reasons you mention.  Jorg Shilly does seem to do things his own 
way....
>
> That being said, here is a review of your patch below.
>
> First, the title should be simply:
>
> 	cdrtools: new package
>
> On Mon,  2 Feb 2015 05:40:52 +0000, Steve Kenton wrote:
>
>> diff --git a/package/Config.in b/package/Config.in
>> index dd011be..1748e88 100644
>> --- a/package/Config.in
>> +++ b/package/Config.in
>> @@ -6,6 +6,7 @@ menu "Audio and video applications"
>>   	source "package/alsa-utils/Config.in"
>>   	source "package/aumix/Config.in"
>>   	source "package/bellagio/Config.in"
>> +	source "package/cdrtools/Config.in"
> Since it's really the same sort of tools than cdrkit, I'd rather see
> cdrtools next to cdrkit in the menus.
OK
>
>> diff --git a/package/cdrtools/Config.in b/package/cdrtools/Config.in
>> new file mode 100644
>> index 0000000..8583cec
>> --- /dev/null
>> +++ b/package/cdrtools/Config.in
>> @@ -0,0 +1,12 @@
>> +config BR2_PACKAGE_CDRTOOLS
>> +	bool "cdrtools"
>> +	depends on BR2_USE_WCHAR&&  BR2_LARGEFILE
> We generally put this on two separate lines.
OK
>
>> +	help
>> +	  Cdrtools is a set of command line programs that
>> +	  allows to record CD/DVD/BluRay media. This
>> +	  package contains the cdrecord&  mkisofs programs.
>> +	
>> +	  http://sourceforge.net/projects/cdrtools
>> +
>> +comment "cdrtools needs a toolchain w/ wchar, largefile"
>> +	depends on !BR2_USE_WCHAR || !BR2_LARGEFILE
>> diff --git a/package/cdrtools/cdrtools.mk b/package/cdrtools/cdrtools.mk
>> new file mode 100644
>> index 0000000..a732e69
>> --- /dev/null
>> +++ b/package/cdrtools/cdrtools.mk
>> @@ -0,0 +1,21 @@
>> +#############################################################
>> +#
>> +# cdrtools
>> +#
>> +#############################################################
> You should have 80 # signs for the header. And one empty new line
> between the header and the first variable.
Oops
>
>> +CDRTOOLS_VERSION = 3.00
>> +CDRTOOLS_SITE = http://sourceforge.net/projects/cdrtools/files
>> +CDRTOOLS_LICENSE = Various parts under GPLv2.0, LGPLv2.1, CDDL - see COPYING
> Don't use "see COPYING", this is already encoded by the
> <pkg>_LICENSE_FILES variable below.
OK, it was strange and I was not sure what the heck to do
>
>> +CDRTOOLS_LICENSE_FILES = COPYING
>> +
>> +define CDRTOOLS_BUILD_CMDS
>> +	$(MAKE) -C $(@D)
>> +endef
> This unfortunately doesn't work at all: it builds the tools with the
> host compiler, so always for x86 or x86-64. Is this your intention? If
> it is your intention, then it should be a host package, and not a
> target package.
I'm using an x86 host and target so it's easy to get them confused.
I'll look into this and the note below and try again

Steve
>> +
>> +define CDRTOOLS_INSTALL_TARGET_CMDS
>> +	$(INSTALL) -m 0755 -D $(@D)/cdrecord/OBJ/x86_64-linux-cc/cdrecord $(TARGET_DIR)/usr/bin/cdrecord
>> +	$(INSTALL) -m 0755 -D $(@D)/readcd/OBJ/x86_64-linux-cc/readcd $(TARGET_DIR)/usr/bin/readcd
>> +	$(INSTALL) -m 0755 -D $(@D)/mkisofs/OBJ/x86_64-linux-cc/mkisofs $(TARGET_DIR)/usr/bin/mkisofs
>> +endef
> This cannot work: you enforce "x86_64-linux-cc", but we might be
> building for ARM, PowerPC, MIPS, etc. Also, isn't there a "make
> install" rule to simplify the installation logic?
>
> So basically, try to do a build for ARM, and make sure 1/ it builds
> fine and 2/ the installed binaries are actually built for ARM.
>
> Thanks!
>
> Thomas