dcmtk: new package

PATCH New William Frost 2 participants 3 messages Jun 18, 2014 – Jul 17, 2014 Arm MediaTek

Share & tools

William Frost

author

2014-06-18 01:33 UTC #0 lore raw

PATCH v4 1/1 dcmtk: new package

[PATCH] Changes v3 -> v4: (suggested by Gustavo Zacarias):
- removed trailing whitespaces in the help text
- added a comment indicating that dcmtk depends on BR2_INSTALL_LIBSTDCPP

Signed-off-by William Frost <[email protected]>

---
 package/Config.in       |  1 +
 package/dcmtk/Config.in | 15 +++++++++++++++
 package/dcmtk/dcmtk.mk  | 23 +++++++++++++++++++++++
 3 files changed, 39 insertions(+)
 create mode 100644 package/dcmtk/Config.in
 create mode 100644 package/dcmtk/dcmtk.mk

package/Config.in +1 -0

index 07fd166..b88c0b0 100644
@@ -568,6 +568,7 @@ endmenu
 menu "Graphics"
 source "package/atk/Config.in"
 source "package/cairo/Config.in"
+source "package/dcmtk/Config.in"
 source "package/fltk/Config.in"
 source "package/fontconfig/Config.in"
 source "package/freetype/Config.in"

package/dcmtk/Config.in new file +15 -0

new file mode 100644
index 0000000..d11bc12
@@ -0,0 +1,15 @@
+config BR2_PACKAGE_DCMTK
+	bool "dcmtk"
+	help
+	  DCMTK is a collection of libraries and applications implementing
+	  large parts the DICOM standard. It includes software for examining,
+	  constructing and converting DICOM image files, handling offline
+	  media, sending and receiving images over a network connection, as
+	  well as demonstrative image storage and worklist servers. DCMTK is
+	  is written in a mixture of ANSI C and C++. It comes in complete
+	  source code and is made available as "open source" software.
+
+	  http://dicom.offis.de/dcmtk.php.en
+
+comment "dcmtk needs a toolchain w/ C++"
+	depends on !BR2_INSTALL_LIBSTDCPP
\ No newline at end of file

package/dcmtk/dcmtk.mk new file +23 -0

new file mode 100644
index 0000000..ae2c265
@@ -0,0 +1,23 @@
+################################################################################
+#
+# dcmtk
+#
+################################################################################
+
+DCMTK_VERSION = $(call qstrip,"3.6.0")
+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/dcmtk360
+
+DCMTK_LICENSE = BSD
+DCMTK_LICENSE_FILES = COPYRIGHT
+DCMTK_INSTALL_STAGING = YES
+
+DCMTK_CFLAGS = $(TARGET_CFLAGS) -O -Wall
+DCMTK_CXXFLAGS = $(TARGET_CXXFLAGS) -O  -Wall
+DCMTK_CONF_OPT = \
+	--disable-rpath --with-zlib \
+	ac_cv_my_c_rightshift_unsigned=no
+
+DCMTK_CONF_ENV = ARFLAGS=cru
+DCMTK_MAKE_OPT	= DESTDIR=$(STAGING_DIR) install-lib
+
+$(eval $(autotools-package))

Signature · 1 line

1.8.1.4

Thomas Petazzoni

2014-06-29 14:07 UTC #1 lore raw

PATCH v4 1/1 dcmtk: new package

Dear William Frost,

Thanks for this patch. Unfortunately, it still contain a number of issues, some minor, other more important.

First, the title of your patch. Apparently, you did some change to add [Patch v4] in the title, which is completely unnecessary. You should instead simply use:

	git format-patch --subject-prefix="PATCH v4" HEAD^

to generate the patch to be sent to the mailing list. This will take care of generating the proper [...] part of the patch title.

Quoted 3 lines · William Frost

[PATCH] Changes v3 -> v4: (suggested by Gustavo Zacarias):
- removed trailing whitespaces in the help text
- added a comment indicating that dcmtk depends on BR2_INSTALL_LIBSTDCPP

As I've already said, this should *not* be part of the commit log. It should instead be...

Quoted 3 lines

Signed-off-by William Frost <[email protected]>
---

here, or part of a separate cover letter. Please read
http://buildroot.org/downloads/manual/manual.html#submitting-patches
carefully to know how to submit patches.

Quoted 25 lines

 package/Config.in       |  1 +
 package/dcmtk/Config.in | 15 +++++++++++++++
 package/dcmtk/dcmtk.mk  | 23 +++++++++++++++++++++++
 3 files changed, 39 insertions(+)
 create mode 100644 package/dcmtk/Config.in
 create mode 100644 package/dcmtk/dcmtk.mk

package/Config.in +1 -0

index 07fd166..b88c0b0 100644
@@ -568,6 +568,7 @@ endmenu
 menu "Graphics"
 source "package/atk/Config.in"
 source "package/cairo/Config.in"
+source "package/dcmtk/Config.in"
 source "package/fltk/Config.in"
 source "package/fontconfig/Config.in"
 source "package/freetype/Config.in"

package/dcmtk/Config.in new file +0 -0

new file mode 100644
index 0000000..d11bc12

package/dcmtk/Config.in line 2

Quoted 2 lines

package/dcmtk/Config.in new file +2 -0
@@ -0,0 +1,15 @@
+config BR2_PACKAGE_DCMTK
+	bool "dcmtk"

Your package needs C++, so you lack a:

depends on BR2_INSTALL_LIBSTDCPP

here.

package/dcmtk/Config.in line 10

Quoted 8 lines

package/dcmtk/Config.in new file +8 -0

+	help
+	  DCMTK is a collection of libraries and applications implementing
+	  large parts the DICOM standard. It includes software for examining,
+	  constructing and converting DICOM image files, handling offline
+	  media, sending and receiving images over a network connection, as
+	  well as demonstrative image storage and worklist servers. DCMTK is
+	  is written in a mixture of ANSI C and C++. It comes in complete
+	  source code and is made available as "open source" software.

Please wrap the help text to a shorter length (72 characters).

package/dcmtk/Config.in line 15

Quoted 5 lines

package/dcmtk/Config.in new file +4 -0

+	  http://dicom.offis.de/dcmtk.php.en
+
+comment "dcmtk needs a toolchain w/ C++"
+	depends on !BR2_INSTALL_LIBSTDCPP
\ No newline at end of file

There should be a new line at the end of the file.

package/dcmtk/dcmtk.mk line 7

Quoted 13 lines

package/dcmtk/dcmtk.mk new file +7 -0

new file mode 100644
index 0000000..ae2c265
@@ -0,0 +1,23 @@
+################################################################################
+#
+# dcmtk
+#
+################################################################################
+
+DCMTK_VERSION = $(call qstrip,"3.6.0")

Just do:

DCMTK_VERSION = 3.6.0

there's absolutely no point in declaring a variable with quotes to remove them right after by calling the 'qstrip' function. It's like doing 2 + 0 + 0 + 0 + 0 + 0 instead of just using 2.

package/dcmtk/dcmtk.mk line 10

Quoted 3 lines

package/dcmtk/dcmtk.mk new file +3 -0

+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/dcmtk360
+
+DCMTK_LICENSE = BSD

BSD-3c

package/dcmtk/dcmtk.mk line 15

Quoted 5 lines

package/dcmtk/dcmtk.mk new file +5 -0

+DCMTK_LICENSE_FILES = COPYRIGHT
+DCMTK_INSTALL_STAGING = YES
+
+DCMTK_CFLAGS = $(TARGET_CFLAGS) -O -Wall
+DCMTK_CXXFLAGS = $(TARGET_CXXFLAGS) -O  -Wall

These two variables are useless. They do not exist in the package infrastructure, and since they are not used below, they do not serve any purpose.

package/dcmtk/dcmtk.mk line 20

Quoted 5 lines

package/dcmtk/dcmtk.mk new file +5 -0

+DCMTK_CONF_OPT = \
+	--disable-rpath --with-zlib \
+	ac_cv_my_c_rightshift_unsigned=no
+
+DCMTK_CONF_ENV = ARFLAGS=cru

This is all mixed up, and makes confusion between configure options and configure variables. Please change to:

DCMTK_CONF_OPT = \
	--disable-rpath \
	--with-zlib

# We define ac_cv_my_c_rightshift_unsigned, because the configure

# script has an AC_TRY_RUN test that is unsuitable for
# cross-compilation. All sane architectures have signed right-shifting,
# so we make the assumption that all architectures supported by
# Buildroot have this behavior.
#
# The configure script only checks if AR is "ar" to decide to pass the
# cru ARFLAGS. In cross-compilation mode AR is "<prefix>-ar", so the
# configure script test doesn't work. Work around this by explicitly
# passing the appropriate ARFLAGS.

DCMTK_CONF_ENV = \
	ac_cv_my_c_rightshift_unsigned=no \
	ARFLAGS=cru

package/dcmtk/dcmtk.mk line 21

Quoted 1 line

package/dcmtk/dcmtk.mk new file +1 -0
+DCMTK_MAKE_OPT	= DESTDIR=$(STAGING_DIR) install-lib

This really shouldn't be used. If something special is needed at install time, use INSTALL_TARGET_OPT or INSTALL_STAGING_OPT. But clearly, using DESTDIR=$(STAGING_DIR) for all steps including the target installation step doesn't make sense.

Also, there are more serious problems with the package:

* It does not build with uClibc, due to issues around isnan() and finite(). I tried a bit to fix them, but what the DCMTK code is doing with math functions is quite scary and I didn't had the time to dive into all the details. Errors are:

ofstd.cc: In function ?int my_isinf(double)?:
ofstd.cc:218:21: error: ?finite? was not declared in this scope ofstd.cc:218:37: error: ?isnan? was not declared in this scope

Note that uClibc support is not mandatory, so if you're not interested in building this package with uClibc, you can add a glibc dependency to your package.

* It also does not build with glibc, with the following output:

../../ofstd/include/dcmtk/ofstd/ofoset.h:149:34: error: 'Resize' was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive] Resize( this->size * 2 );

* The configure script mistakenly detects some host libraries, such as libxml2 in my case, and therefore uses incorrect header path:

cc1plus: warning: include location "/usr/include/libxml2" is unsafe for cross-compilation [-Wpoison-system-directories]

To avoid that, you should have a look at *all* the configure script options related to optional libraries that DCMTK can use, and either disable their usage, or support their usage in your dcmtk.mk file. So either:

DCMTK_CONF_OPT = \
	--without-openssl \
	--without-zlib \
	--without-libtiff \
	--without-libpng \
	--without-libxml \
	--without-libwrap \
	--without-libsndfile

Or, if you want to support the libraries, for each library, something like:

ifeq ($(BR2_PACKAGE_<library>),y)
DCMTK_CONF_OPT += --with-<library> --with-<library>inc=$(STAGING_DIR) DCMTK_DEPENDENCIES += <library>
else
DCMTK_CONF_OPT += --without-<library>
endif

These solutions will ensure that the DCMTK configure script will not mistakenly thing a certain library is installed due to it being available on the build machine.

Could you fix those issues and resend?

Thanks,

Thomas Petazzoni

Signature · 3 lines

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

Thomas Petazzoni

2014-07-17 19:05 UTC #2 lore raw

PATCH v4 1/1 dcmtk: new package

William,

Have you seen my review of your dcmtk patch below? Do you intend to send an updated version of the patch that solves the various problems raised in this review?

It would be great, because your patch is currently waiting in our patch tracking system, but we cannot apply it as is due to these issues.

Thanks a lot for your contribution!

Thomas

Quoted 216 lines · Thomas Petazzoni

Dear William Frost,
Thanks for this patch. Unfortunately, it still contain a number of issues, some minor, other more important.
First, the title of your patch. Apparently, you did some change to add [Patch v4] in the title, which is completely unnecessary. You should instead simply use:
	git format-patch --subject-prefix="PATCH v4" HEAD^
to generate the patch to be sent to the mailing list. This will take care of generating the proper [...] part of the patch title.
Quoted 3 lines · William Frost
[PATCH] Changes v3 -> v4: (suggested by Gustavo Zacarias):
- removed trailing whitespaces in the help text
- added a comment indicating that dcmtk depends on BR2_INSTALL_LIBSTDCPP
As I've already said, this should *not* be part of the commit log. It should instead be...
Quoted 3 lines
Signed-off-by William Frost <[email protected]>
---
here, or part of a separate cover letter. Please read
http://buildroot.org/downloads/manual/manual.html#submitting-patches
carefully to know how to submit patches.
Quoted 25 lines
 package/Config.in       |  1 +
 package/dcmtk/Config.in | 15 +++++++++++++++
 package/dcmtk/dcmtk.mk  | 23 +++++++++++++++++++++++
 3 files changed, 39 insertions(+)
 create mode 100644 package/dcmtk/Config.in
 create mode 100644 package/dcmtk/dcmtk.mk
package/Config.in +1 -0
index 07fd166..b88c0b0 100644
@@ -568,6 +568,7 @@ endmenu
 menu "Graphics"
 source "package/atk/Config.in"
 source "package/cairo/Config.in"
+source "package/dcmtk/Config.in"
 source "package/fltk/Config.in"
 source "package/fontconfig/Config.in"
 source "package/freetype/Config.in"
package/dcmtk/Config.in new file +0 -0
new file mode 100644
index 0000000..d11bc12
package/dcmtk/Config.in line 2
Quoted 2 lines
package/dcmtk/Config.in new file +2 -0
@@ -0,0 +1,15 @@
+config BR2_PACKAGE_DCMTK
+	bool "dcmtk"
Your package needs C++, so you lack a:
depends on BR2_INSTALL_LIBSTDCPP
here.
package/dcmtk/Config.in line 10
Quoted 8 lines
package/dcmtk/Config.in new file +8 -0
+	help
+	  DCMTK is a collection of libraries and applications implementing
+	  large parts the DICOM standard. It includes software for examining,
+	  constructing and converting DICOM image files, handling offline
+	  media, sending and receiving images over a network connection, as
+	  well as demonstrative image storage and worklist servers. DCMTK is
+	  is written in a mixture of ANSI C and C++. It comes in complete
+	  source code and is made available as "open source" software.
Please wrap the help text to a shorter length (72 characters).
package/dcmtk/Config.in line 15
Quoted 5 lines
package/dcmtk/Config.in new file +4 -0
+	  http://dicom.offis.de/dcmtk.php.en
+
+comment "dcmtk needs a toolchain w/ C++"
+	depends on !BR2_INSTALL_LIBSTDCPP
\ No newline at end of file
There should be a new line at the end of the file.
package/dcmtk/dcmtk.mk line 7
Quoted 13 lines
package/dcmtk/dcmtk.mk new file +7 -0
new file mode 100644
index 0000000..ae2c265
@@ -0,0 +1,23 @@
+################################################################################
+#
+# dcmtk
+#
+################################################################################
+
+DCMTK_VERSION = $(call qstrip,"3.6.0")
Just do:
DCMTK_VERSION = 3.6.0
there's absolutely no point in declaring a variable with quotes to remove them right after by calling the 'qstrip' function. It's like doing 2 + 0 + 0 + 0 + 0 + 0 instead of just using 2.
package/dcmtk/dcmtk.mk line 10
Quoted 3 lines
package/dcmtk/dcmtk.mk new file +3 -0
+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/dcmtk360
+
+DCMTK_LICENSE = BSD
BSD-3c
package/dcmtk/dcmtk.mk line 15
Quoted 5 lines
package/dcmtk/dcmtk.mk new file +5 -0
+DCMTK_LICENSE_FILES = COPYRIGHT
+DCMTK_INSTALL_STAGING = YES
+
+DCMTK_CFLAGS = $(TARGET_CFLAGS) -O -Wall
+DCMTK_CXXFLAGS = $(TARGET_CXXFLAGS) -O  -Wall
These two variables are useless. They do not exist in the package infrastructure, and since they are not used below, they do not serve any purpose.
package/dcmtk/dcmtk.mk line 20
Quoted 5 lines
package/dcmtk/dcmtk.mk new file +5 -0
+DCMTK_CONF_OPT = \
+	--disable-rpath --with-zlib \
+	ac_cv_my_c_rightshift_unsigned=no
+
+DCMTK_CONF_ENV = ARFLAGS=cru
This is all mixed up, and makes confusion between configure options and configure variables. Please change to:
DCMTK_CONF_OPT = \
	--disable-rpath \
	--with-zlib
# We define ac_cv_my_c_rightshift_unsigned, because the configure
# script has an AC_TRY_RUN test that is unsuitable for
# cross-compilation. All sane architectures have signed right-shifting,
# so we make the assumption that all architectures supported by
# Buildroot have this behavior.
#
# The configure script only checks if AR is "ar" to decide to pass the
# cru ARFLAGS. In cross-compilation mode AR is "<prefix>-ar", so the
# configure script test doesn't work. Work around this by explicitly
# passing the appropriate ARFLAGS.
DCMTK_CONF_ENV = \
	ac_cv_my_c_rightshift_unsigned=no \
	ARFLAGS=cru
package/dcmtk/dcmtk.mk line 21
Quoted 1 line
package/dcmtk/dcmtk.mk new file +1 -0
+DCMTK_MAKE_OPT	= DESTDIR=$(STAGING_DIR) install-lib
This really shouldn't be used. If something special is needed at install time, use INSTALL_TARGET_OPT or INSTALL_STAGING_OPT. But clearly, using DESTDIR=$(STAGING_DIR) for all steps including the target installation step doesn't make sense.
Also, there are more serious problems with the package:
* It does not build with uClibc, due to issues around isnan() and finite(). I tried a bit to fix them, but what the DCMTK code is doing with math functions is quite scary and I didn't had the time to dive into all the details. Errors are:
ofstd.cc: In function ?int my_isinf(double)?:
ofstd.cc:218:21: error: ?finite? was not declared in this scope ofstd.cc:218:37: error: ?isnan? was not declared in this scope
Note that uClibc support is not mandatory, so if you're not interested in building this package with uClibc, you can add a glibc dependency to your package.
* It also does not build with glibc, with the following output:
../../ofstd/include/dcmtk/ofstd/ofoset.h:149:34: error: 'Resize' was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive] Resize( this->size * 2 );
* The configure script mistakenly detects some host libraries, such as libxml2 in my case, and therefore uses incorrect header path:
cc1plus: warning: include location "/usr/include/libxml2" is unsafe for cross-compilation [-Wpoison-system-directories]
To avoid that, you should have a look at *all* the configure script options related to optional libraries that DCMTK can use, and either disable their usage, or support their usage in your dcmtk.mk file. So either:
DCMTK_CONF_OPT = \
	--without-openssl \
	--without-zlib \
	--without-libtiff \
	--without-libpng \
	--without-libxml \
	--without-libwrap \
	--without-libsndfile
Or, if you want to support the libraries, for each library, something like:
ifeq ($(BR2_PACKAGE_<library>),y)
DCMTK_CONF_OPT += --with-<library> --with-<library>inc=$(STAGING_DIR) DCMTK_DEPENDENCIES += <library>
else
DCMTK_CONF_OPT += --without-<library>
endif
These solutions will ensure that the DCMTK configure script will not mistakenly thing a certain library is installed due to it being available on the build machine.
Could you fix those issues and resend?
Thanks,
Thomas Petazzoni

Signature · 3 lines

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