Skip to contentSkip to conversation

DCMTK: new package

PATCH Superseded William Frost 2 participants 3 messages Apr 28, 2014 – Jun 12, 2014 Arm MediaTek

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 DCMTK: new package

It works fine but still every time I type "make all" in Buildroot dcmtk make the configure process again. Any idea on how to solve this?

---
Changes v1 -> v2:
- fixed DCMTK_LICENSE to BDS
- changed --host=$(arm-none-linux-gnueabi) to --host=$(GNU_TARGET_NAME)
- fixed PKG_CONFIG_PATH and PKG_CONFIG_SYSROOT_DIR

(suggested by Thomas De Schampheleire):
- fixed all lines to less than 80 characters in Config.in ans dcmtk.mk
- changed BR2_DCMTK_VERSION to BR2_PACKAGE_DCMTK_VERSION
- removed DCMTK_SOURCE

---
  package/Config.in       |  1 +
  package/dcmtk/Config.in | 38 ++++++++++++++++++++++
  package/dcmtk/dcmtk.mk  | 85 
+++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 124 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 +38 -0
new file mode 100644
index 0000000..ba9d2ee
@@ -0,0 +1,38 @@
+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
+
+if BR2_PACKAGE_DCMTK
+
+choice
+    prompt "DCMTK Version"
+    default BR2_PACKAGE_DCMTK_VERSION_3_6_0
+    help
+      Select the version of DCMTK you wish to use.
+
+    config BR2_PACKAGE_DCMTK_VERSION_3_6_0
+        bool "DCMTK 3.6.0"
+
+    config BR2_PACKAGE_DCMTK_VERSION_SNAPSHOT_2012
+        bool "DCMTK 3.6.1 snapshot (2012.11.02)"
+
+    config BR2_PACKAGE_DCMTK_VERSION_SNAPSHOT_2013
+        bool "DCMTK 3.6.1 snapshot (2013.11.14)"
+
+endchoice
+
+config BR2_PACKAGE_DCMTK_VERSION
+    string
+    default "3.6.0"    if BR2_PACKAGE_DCMTK_VERSION_3_6_0
+    default "3.6.1_20121102" if BR2_PACKAGE_DCMTK_VERSION_SNAPSHOT_2012
+    default "3.6.1_20131114" if BR2_PACKAGE_DCMTK_VERSION_SNAPSHOT_2013
+endif
package/dcmtk/dcmtk.mk new file +27 -0
new file mode 100644
index 0000000..5c9a851
@@ -0,0 +1,85 @@
+################################################################################
+#
+# dcmtk
+#
+################################################################################
+DCMTK_VERSION = $(call qstrip,$(BR2_PACKAGE_DCMTK_VERSION))
+
+ifeq ($(BR2_PACKAGE_DCMTK_VERSION), "3.6.0")
+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/dcmtk360/
+endif
+ifeq ($(BR2_PACKAGE_DCMTK_VERSION),"3.6.1_20121102")
+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/snapshot/old
+endif
+ifeq ($(BR2_PACKAGE_DCMTK_VERSION),"3.6.1_20131114")
+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/snapshot
+endif
+
+DCMTK_LICENSE = BSD
+DCMTK_LICENSE_FILES = COPYING
+DCMTK_INSTALL_STAGING = YES
+
+DCMTK_CFLAGS = $(TARGET_CFLAGS) --sysroot=$(STAGING_DIR) -O -D_REENTRANT \
+        -D_XOPEN_SOURCE_EXTENDED -D_XOPEN_SOURCE=500 -D_BSD_SOURCE \
+        -D_BSD_COMPAT -D_OSF_SOURCE -D_POSIX_C_SOURCE=199506L -Wall \
+        -Wno-psabi
+
+DCMTK_CXXFLAGS = $(TARGET_CFLAGS) --sysroot=$(STAGING_DIR) -O 

-D_REENTRANT \

+        -D_XOPEN_SOURCE_EXTENDED -D_XOPEN_SOURCE=500 -D_BSD_SOURCE \
+        -D_BSD_COMPAT -D_OSF_SOURCE -D_POSIX_C_SOURCE=199506L -Wall \
+        -Wno-psabi

+
+DCMTK_CPPFLAGS = $(TARGET_CFLAGS) --sysroot=$(STAGING_DIR) +DCMTK_LDFLAGS = $(TARGET_CFLAGS) --sysroot=$(STAGING_DIR)
+
+DCMTK_INSTALL_STAGING_OPT = DESTDIR=$(STAGING_DIR) STRIP=$(TARGET_STRIP) \
+ install
+
+DCMTK_INSTALL_TARGET_OPT = DESTDIR=$(TARGET_DIR) STRIP=$(TARGET_STRIP) install
+
+DCMTK_CONF_OPT = --without-libtiff --without-openssl --without-libxml \
+ --without-libpng --without-libsndfile --disable-debug \
+ --without-private-tags --enable-std-includes --disable-rpath
+
+ifeq ($(BR2_PACKAGE_ZLIB),y)
+ DCMTK_DEPENDENCIES += zlib
+else
+ DCMTK_CONF_OPT += --without-zlib
+endif
+VERBOSE=1
+
+define DCMTK_CONFIG_SET
+ $(CAT) $(@D)/config/Makefile.def | $(SED) 's%^$(1).*%$(1) = $(2)%g' \
+ $(@D)/config/Makefile.def
+endef
+
+define DCMTK_CONFIGURE_CMDS
+ (cd $(@D); \

+        CFLAGS="$(DCMTK_CFLAGS)" \
+        CXXFLAGS="$(DCMTK_CXXFLAGS)" \
+        CPPFLAGS="$(DCMTK_CPPFLAGS)" \
+        LDFLAGS="$(DCMTK_LDFLAGS)" \
+        ARFLAGS=cru \
+        CC=$(TARGET_CC) \
+        CXX=$(TARGET_CXX) \
+        RANLIB=$(TARGET_RANLIB) \
+        AR=$(TARGET_AR) \
+        STRIP=$(TARGET_STRIP) \
+        PKG_CONFIG_PATH="$(STAGING_DIR)/usr/lib/pkgconfig" \
+        PKG_CONFIG_SYSROOT_DIR="$(STAGING_DIR)" \
+        PKG_CONFIG="$(PKG_CONFIG_HOST_BINARY)" \
+        MAKEFLAGS="$(MAKEFLAGS) -j$(PARALLEL_JOBS)" ./configure \
+        $(if $(VERBOSE),-verbose,-silent) \
+        $(DCMTK_CONF_OPT) \
+        ac_cv_my_c_rightshift_unsigned=no --prefix=$(STAGING_DIR)/usr \
+        --with-zlibinc=$(STAGING_DIR)/usr \
+        --host=$(GNU_TARGET_NAME) \

+ )
+endef
+
+define DCMTK_BUILD_CMDS
+ $(call DCMTK_CONFIG_SET,"CC =",$(TARGET_CC))
+ $(MAKE) -C $(@D) install-lib
+endef
+
+$(eval $(autotools-package))

Signature · 1 line
1.8.1.4
#1 lore raw

PATCH v2 1/1 DCMTK: new package

Dear William Frost,

Quoted 2 lines · William Frost

It works fine but still every time I type "make all" in Buildroot dcmtk make the configure process again. Any idea on how to solve this?

This message shouldn't be part of the commit log. If you want to give such "side" messages, do it below the "---" sign with the changelog, or in a separate cover letter.

The title of the patch should have DCMTK in lower case, for consistency with what we do for other packages.

Also, you're still using Thunderbird to post your patches, and they are badly line-wrapped. It might also be the reason why the Config.in file indentation is wrong, so I won't comment on this for now, waiting for you to resend a new version with git send-email.

Quoted 14 lines
package/dcmtk/Config.in new file +14 -0
+if BR2_PACKAGE_DCMTK
+
+choice
+    prompt "DCMTK Version"
+    default BR2_PACKAGE_DCMTK_VERSION_3_6_0
+    help
+      Select the version of DCMTK you wish to use.
+
+    config BR2_PACKAGE_DCMTK_VERSION_3_6_0
+        bool "DCMTK 3.6.0"
+
+    config BR2_PACKAGE_DCMTK_VERSION_SNAPSHOT_2012
+        bool "DCMTK 3.6.1 snapshot (2012.11.02)"
+

package/dcmtk/Config.in line 37

Quoted 10 lines
package/dcmtk/Config.in new file +10 -0
+    config BR2_PACKAGE_DCMTK_VERSION_SNAPSHOT_2013
+        bool "DCMTK 3.6.1 snapshot (2013.11.14)"
+
+endchoice
+
+config BR2_PACKAGE_DCMTK_VERSION
+    string
+    default "3.6.0"    if BR2_PACKAGE_DCMTK_VERSION_3_6_0
+    default "3.6.1_20121102" if BR2_PACKAGE_DCMTK_VERSION_SNAPSHOT_2012
+    default "3.6.1_20131114" if BR2_PACKAGE_DCMTK_VERSION_SNAPSHOT_2013

Do we really need to make the version configurable? There are *very* few packages for which we support that, and we generally try to avoid this as much as possible.

package/dcmtk/dcmtk.mk line 6

Quoted 12 lines
package/dcmtk/dcmtk.mk new file +6 -0
new file mode 100644
index 0000000..5c9a851
@@ -0,0 +1,85 @@
+################################################################################
+#
+# dcmtk
+#
+################################################################################
+DCMTK_VERSION = $(call qstrip,$(BR2_PACKAGE_DCMTK_VERSION))

One empty new line between the comment header and the first variable definition.

package/dcmtk/dcmtk.mk line 18

Quoted 12 lines
package/dcmtk/dcmtk.mk new file +12 -0
+
+ifeq ($(BR2_PACKAGE_DCMTK_VERSION), "3.6.0")
+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/dcmtk360/
+endif
+ifeq ($(BR2_PACKAGE_DCMTK_VERSION),"3.6.1_20121102")
+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/snapshot/old
+endif
+ifeq ($(BR2_PACKAGE_DCMTK_VERSION),"3.6.1_20131114")
+DCMTK_SITE = http://dicom.offis.de/download/dcmtk/snapshot
+endif
+
+DCMTK_LICENSE = BSD

The license looks more complex than that. Maybe you want to seek the input of Luca and/or Yann on this.

package/dcmtk/dcmtk.mk line 19

Quoted 1 line
package/dcmtk/dcmtk.mk new file +1 -0
+DCMTK_LICENSE_FILES = COPYING

There is no file named 'COPYING' in the source code, so this cannot work. Maybe you want to use the 'COPYRIGHT' file instead.

package/dcmtk/dcmtk.mk line 25

Quoted 6 lines
package/dcmtk/dcmtk.mk new file +6 -0
+DCMTK_INSTALL_STAGING = YES
+
+DCMTK_CFLAGS = $(TARGET_CFLAGS) --sysroot=$(STAGING_DIR) -O -D_REENTRANT \
+        -D_XOPEN_SOURCE_EXTENDED -D_XOPEN_SOURCE=500 -D_BSD_SOURCE \
+        -D_BSD_COMPAT -D_OSF_SOURCE -D_POSIX_C_SOURCE=199506L -Wall \
+        -Wno-psabi

This looks suspicious. First, the --sysroot option is for sure not needed. And all the other -D options look weird. Why are they needed?

package/dcmtk/dcmtk.mk line 27

Quoted 1 line
package/dcmtk/dcmtk.mk new file +1 -0
+DCMTK_CXXFLAGS = $(TARGET_CFLAGS) --sysroot=$(STAGING_DIR) -O 

So you start of TARGET_CFLAGS to define CXXFLAGS ?

package/dcmtk/dcmtk.mk

Quoted 7 lines
package/dcmtk/dcmtk.mk new file +6 -1
-D_REENTRANT \
+        -D_XOPEN_SOURCE_EXTENDED -D_XOPEN_SOURCE=500 -D_BSD_SOURCE \
+        -D_BSD_COMPAT -D_OSF_SOURCE -D_POSIX_C_SOURCE=199506L -Wall \
+        -Wno-psabi
+
+DCMTK_CPPFLAGS = $(TARGET_CFLAGS) --sysroot=$(STAGING_DIR)
+DCMTK_LDFLAGS = $(TARGET_CFLAGS) --sysroot=$(STAGING_DIR)

Not needed, and actually wrong: you use TARGET_CFLAGS to derive your CPPFLAGS and LDFLAGS.

Quoted 5 lines
package/dcmtk/dcmtk.mk new file +4 -0
+DCMTK_INSTALL_STAGING_OPT = DESTDIR=$(STAGING_DIR) STRIP=$(TARGET_STRIP) \
+                install
+
+DCMTK_INSTALL_TARGET_OPT = DESTDIR=$(TARGET_DIR) STRIP=$(TARGET_STRIP) 

install

This is probably not needed.

package/dcmtk/dcmtk.mk

Quoted 7 lines
package/dcmtk/dcmtk.mk new file +7 -0
+
+DCMTK_CONF_OPT = --without-libtiff --without-openssl --without-libxml \
+          --without-libpng --without-libsndfile --disable-debug \
+          --without-private-tags --enable-std-includes --disable-rpath
+
+ifeq ($(BR2_PACKAGE_ZLIB),y)
+    DCMTK_DEPENDENCIES += zlib

DCMTK_CONF_OPT += --with-zlib

package/dcmtk/dcmtk.mk

Quoted 4 lines
package/dcmtk/dcmtk.mk new file +4 -0
+else
+    DCMTK_CONF_OPT += --without-zlib
+endif
+VERBOSE=1

Huh?

package/dcmtk/dcmtk.mk

Quoted 5 lines
package/dcmtk/dcmtk.mk new file +5 -0
+
+define DCMTK_CONFIG_SET
+    $(CAT) $(@D)/config/Makefile.def | $(SED) 's%^$(1).*%$(1) = $(2)%g' \
+    $(@D)/config/Makefile.def
+endef

This would probably require a comment on top of it.

Quoted 14 lines
package/dcmtk/dcmtk.mk new file +14 -0
+
+define DCMTK_CONFIGURE_CMDS
+    (cd $(@D); \
+        CFLAGS="$(DCMTK_CFLAGS)" \
+        CXXFLAGS="$(DCMTK_CXXFLAGS)" \
+        CPPFLAGS="$(DCMTK_CPPFLAGS)" \
+        LDFLAGS="$(DCMTK_LDFLAGS)" \
+        ARFLAGS=cru \
+        CC=$(TARGET_CC) \
+        CXX=$(TARGET_CXX) \
+        RANLIB=$(TARGET_RANLIB) \
+        AR=$(TARGET_AR) \
+        STRIP=$(TARGET_STRIP) \
+        PKG_CONFIG_PATH="$(STAGING_DIR)/usr/lib/pkgconfig" \

package/dcmtk/dcmtk.mk

Quoted 10 lines
package/dcmtk/dcmtk.mk new file +10 -0
+        PKG_CONFIG_SYSROOT_DIR="$(STAGING_DIR)" \
+        PKG_CONFIG="$(PKG_CONFIG_HOST_BINARY)" \
+        MAKEFLAGS="$(MAKEFLAGS) -j$(PARALLEL_JOBS)" ./configure \
+        $(if $(VERBOSE),-verbose,-silent) \
+        $(DCMTK_CONF_OPT) \
+        ac_cv_my_c_rightshift_unsigned=no --prefix=$(STAGING_DIR)/usr \
+        --with-zlibinc=$(STAGING_DIR)/usr \
+        --host=$(GNU_TARGET_NAME) \
+    )
+endef

Clearly, no. If your package uses the autotools build system, the default configure command of the autotools-package infrastructure should work fine, there's no need to duplicate it here. You can pass environment variables using DCMTK_CONF_ENV, and configuration options through DCMTK_CONF_OPT.

That being said, the dcmtk build system looks weird: the main configure script is not the one generated by autoconf. So you're maybe not in the canonical case of an autotools based package.

package/dcmtk/dcmtk.mk

Quoted 5 lines
package/dcmtk/dcmtk.mk new file +5 -0
+
+define DCMTK_BUILD_CMDS
+    $(call DCMTK_CONFIG_SET,"CC =",$(TARGET_CC))
+    $(MAKE) -C $(@D) install-lib
+endef

Same thing here, we normally don't want to duplicate the build command definition, unless there's a really strong reason to do this.

Quoted 2 lines

+
+$(eval $(autotools-package))

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#2 lore raw

PATCH v2 1/1 DCMTK: new package

In reply to William Frost

Dear William Frost,

Quoted 2 lines · William Frost

It works fine but still every time I type "make all" in Buildroot dcmtk make the configure process again. Any idea on how to solve this?

You have sent this patch on April, 29th, and received a number of review comments on the same day. However, you have not resubmitted a new version of your patch adding the DCMTK package that takes into account the comments.

Are you still interested in getting the DCMTK package integrated in Buildroot? Your contribution would definitely be welcome, but we cannot keep patches pending if they are not being worked on until they are merged.

Thanks for your contribution!

Thomas

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