From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Petazzoni Date: Thu, 17 Jul 2014 21:05:48 +0200 Subject: [Buildroot] [PATCH] [Patch v4] dcmtk: new package In-Reply-To: <20140629160710.0fd91b1a@free-electrons.com> References: <1403055183-3910-1-git-send-email-tsmrnd0@gmail.com> <20140629160710.0fd91b1a@free-electrons.com> Message-ID: <20140717210548.3db0c251@free-electrons.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: buildroot@busybox.net 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 On Sun, 29 Jun 2014 16:07:10 +0200, Thomas Petazzoni wrote: > 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. > > On Wed, 18 Jun 2014 10:33:03 +0900, William Frost wrote: > > [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... > > > > > Signed-off-by: William Frost > > --- > > 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. > > > 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 > > > > diff --git a/package/Config.in b/package/Config.in > > index 07fd166..b88c0b0 100644 > > --- a/package/Config.in > > +++ b/package/Config.in > > @@ -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" > > diff --git a/package/dcmtk/Config.in b/package/dcmtk/Config.in > > new file mode 100644 > > index 0000000..d11bc12 > > --- /dev/null > > +++ b/package/dcmtk/Config.in > > @@ -0,0 +1,15 @@ > > +config BR2_PACKAGE_DCMTK > > + bool "dcmtk" > > Your package needs C++, so you lack a: > > depends on BR2_INSTALL_LIBSTDCPP > > here. > > > + 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). > > > + 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. > > > diff --git a/package/dcmtk/dcmtk.mk b/package/dcmtk/dcmtk.mk > > new file mode 100644 > > index 0000000..ae2c265 > > --- /dev/null > > +++ b/package/dcmtk/dcmtk.mk > > @@ -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. > > > +DCMTK_SITE = http://dicom.offis.de/download/dcmtk/dcmtk360 > > + > > +DCMTK_LICENSE = BSD > > BSD-3c > > > +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. > > > +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 "-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 > > > +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_),y) > DCMTK_CONF_OPT += --with- --with-inc=$(STAGING_DIR) > DCMTK_DEPENDENCIES += > else > DCMTK_CONF_OPT += --without- > 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 -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com