[bug#60640] Gnu: Add gdcm

Tor-björn Claesson 写道：
> 1. I have not yet been able to build the bin output,
> which consists of utilities for manipulating DICOM data. 

OK, I'll take a look.

> 2. It does not build pdf-documentation, since that would require
> texlive as a native input.

OK.  You can add this sort of information as a comment by the 
relevant line, so it doesn't get lost (even during review :-).

> 3. It does not perform tests.

OK, I'll take a look.

If tests are disabled, the reason should always be noted in a 
comment.  Even if it's just ‘; no test suite’.

> 4. It uses socketxx, ljpeg and papyrus3 from the gdcm sources.

Here too I've punted on that by just adding a comment for now.

> +(define-public gdcm

It used to be common to unconditionally add packages to the end of 
files, but this needlessly increased the risk of merge conflicts.

Instead, just add them wherever they first fit alphabetically; 
here, I put it above ‘mia’.

> +    (version "3.0.20")

‘guix lint’ says this can be updated to 3.1.0 but I didn't try, as 
I'd rather it be tested by an actual user — i.e., you.

> + 
> "1w78cmm9q7aavs7svdkl4dgilcqk4yazci9m6x7icrssb7cj991i"))))
> +    (build-system cmake-build-system)
> +    (outputs '("out" "doc"))

/share/doc wasn't actually installed into "doc", but to "out", so 
I set the GDCM_INSTALL_DOC_DIR configure flag.

> +    (arguments
> +     (list #:tests? #f
> +           #:phases #~(modify-phases %standard-phases
> +                        (add-before 'configure 'set-LDFLAGS
> +                          (lambda* (#:key inputs outputs 
> #:allow-other-keys)
> +                            (setenv "LDFLAGS"
> +                                    (string-append 
> "-Wl,-rpath="
> +                                                   #$output 
> "/lib"))))
> +                        (add-before 'build 'patch-gdcm-charls.h
> +                          (lambda _
> +                            (substitute* 
> "../source/Utilities/gdcm_charls.h"
> +                              (("# include <CharLS/charls.h>")

Purely as a matter of taste I dropped the ‘# include ’ from both 
strings and escaped the ‘.’ in the regexp.

> +                               "# include <charls/charls.h>")) 
> #t)))

‘#t’ endings are also obsolete.  Just drop them entirely.  Phases 
can now safely return anything, including nothing or undefined.

I added the following phase to work around log spam, since I 
didn't find its source (nor did I look very hard) [edit: it was 
indeed graphviz, thanks].  By default, $HOME is not writable in 
the build environment.

  (add-before 'build 'set-HOME
    ;; The build spams ‘Fontconfig error: No writable cache
    ;; directories’ in a seemingly endless loop otherwise.
    (lambda _
      (setenv "HOME" "/tmp")))

> +           #:configure-flags #~(list 
> "-DCMAKE_SKIP_RPATH:BOOL=YES"

I, opinionated, added newlines after #:phases and 
#:configure-flags.

Some people like the ‘extreme indentation’ you get by throwing 
away half of your screen width.  I find it leads to cramped code 
and noisy patches once the phases need to get actual work done or 
an even longer CMAKE_ flag comes along.

I also added some newlines and tried to group related flags.

> + 
> "-DCMAKE_C_FLAGS=-fvisibility=hidden"
> + 
> "-DCMAKE_CXX_FLAGS=-fvisibility=hidden"

Should these be explained in a very brief comment?

> + 
> "-DGDCM_USE_SYSTEM_EXPAT:BOOL=ON"
> + 
> "-DGDCM_USE_SYSTEM_ZLIB:BOOL=ON"
> + 
> "-DGDCM_USE_SYSTEM_CHARLS:BOOL=ON"
> + 
> "-DGDCM_USE_SYSTEM_POPPLER:BOOL=ON"
> + 
> "-DGDCM_USE_SYSTEM_LIBXML2:BOOL=ON"
> + 
> "-DGDCM_USE_SYSTEM_JSON:BOOL=ON"
> + 
> "-DGDCM_USE_SYSTEM_UUID:BOOL=ON"
> + 
> "-DGDCM_USE_SYSTEM_OPENJPEG:BOOL=ON"
> + 
> "-DGDCM_USE_SYSTEM_OPENSSL:BOOL=ON"

Thank you for building with system libraries!  Also remove the 
bundled copies when possible.  I did so in a (rather strict) 
source snippet.

> + 
> "-DGDCM_PDF_DOCUMENTATION:BOOL=OFF"

I cannot get the man pages to build, either.  They need something 
called ‘xsl-ns’.  I've disabled GDCM_BUILD_DOCBOOK_MANPAGES for 
now.

> + 
> "-DGCM_BUILD_TESTING:BOOL=OFF"

Why is this set?  It's reported by CMake as having no effect, and 
a diff of the output confirms that.

> + 
> "-DGDCM_BUILD_APPLICATIONS:BOOL=OFF"

I added a ‘TODO’ comment above these bundled projects:

> + 
> "-DGDCM_USE_SYSTEM_PAPYRUS3:BOOL=OFF"
> + 
> "-DGDCM_USE_SYSTEM_SOCKETXX:BOOL=OFF"
> + 
> "-DGDCM_USE_SYSTEM_LJPEG:BOOL=OFF")))
> +    (inputs (list openssl
> +                  expat
> +                  charls
> +                  poppler
> +                  libxml2
> +                  json-c
> +                  openjpeg
> +                  `(,util-linux "lib")
> +                  zlib))
> +    (native-inputs (list git pkg-config doxygen graphviz))

Sorted both.

> +    (home-page "https://gdcm.sourceforge.net")
> +    (synopsis
> +     "C++ library dedicated to reading/parsing and writing 
> DICOM medical files")
> +    (description
> +     "Grassroots DICOM (GDCM) is an implementation of the DICOM 
> standard
> +designed to be open source so that researchers may access 
> clinical data
> +directly.  GDCM includes a file format definition and a network 
> communications
> +protocol, both of which should be extended to provide a full 
> set of tools for
> +a researcher or small medical imaging vendor to interface with 
> an existing
> +medical database.")

I rewrote this just a smidge.  Mostly to remove the extra words, 
and use pretty @acronym{} mark-up.

> +    (license license:bsd-3)))

I still need to check this.

I've attached my WIP V2 to this message, with a commit message 
matching our conventions.

Kind regards,

T G-R

Hi!

Tobias Geerinckx-Rice <me@tobias.gr> writes:
>
>> 3. It does not perform tests.
>
> OK, I'll take a look.
>
> If tests are disabled, the reason should always be noted in a comment.
> Even if it's just ‘; no test suite’.
>

#:tests? #t makes the build fail with "make: *** No rule to make target
 'test'.  Stop."
 
GDCM has nightly regression tests
(https://open.cdash.org/index.php?project=GDCM), should we try to run
those when building? I have tried to find out how to do this but for now
with no success. Maybe it is obvious to more experienced people?

>> +(define-public gdcm
>
> It used to be common to unconditionally add packages to the end of
> files, but this needlessly increased the risk of merge conflicts.
>
> Instead, just add them wherever they first fit alphabetically; here, I
> put it above ‘mia’.
>

Ok, will do from now on!

>> +    (version "3.0.20")
>
> ‘guix lint’ says this can be updated to 3.1.0 but I didn't try, as I'd
> rather it be tested by an actual user — i.e., you.
>

I got that too, but the latest release in git is 3.0.20

>> + "1w78cmm9q7aavs7svdkl4dgilcqk4yazci9m6x7icrssb7cj991i"))))
>> +    (build-system cmake-build-system)
>> +    (outputs '("out" "doc"))
>
> /share/doc wasn't actually installed into "doc", but to "out", so I
> set the GDCM_INSTALL_DOC_DIR configure flag.
>
>> +    (arguments
>> +     (list #:tests? #f
>> +           #:phases #~(modify-phases %standard-phases
>> +                        (add-before 'configure 'set-LDFLAGS
>> +                          (lambda* (#:key inputs outputs
>> #:allow-other-keys)
>> +                            (setenv "LDFLAGS"
>> +                                    (string-append "-Wl,-rpath="
>> +                                                   #$output
>> "/lib"))))
>> +                        (add-before 'build 'patch-gdcm-charls.h
>> +                          (lambda _
>> +                            (substitute*
>> "../source/Utilities/gdcm_charls.h"
>> +                              (("# include <CharLS/charls.h>")

Ah, good catch!

>
> Purely as a matter of taste I dropped the ‘# include ’ from both
> strings and escaped the ‘.’ in the regexp.
>
>> +                               "# include <charls/charls.h>"))
>> #t)))
>
> ‘#t’ endings are also obsolete.  Just drop them entirely.  Phases can
> now safely return anything, including nothing or undefined.
>
> I added the following phase to work around log spam, since I didn't
> find its source (nor did I look very hard) [edit: it was indeed
> graphviz, thanks].  By default, $HOME is not writable in the build
> environment.
>
>  (add-before 'build 'set-HOME
>    ;; The build spams ‘Fontconfig error: No writable cache
>    ;; directories’ in a seemingly endless loop otherwise.
>    (lambda _
>      (setenv "HOME" "/tmp")))
>
>> +           #:configure-flags #~(list "-DCMAKE_SKIP_RPATH:BOOL=YES"

Is this needed, btw? It came from gdcm:s packaging
instructions. Removing it causes no verify-runpath issues.

>
> I, opinionated, added newlines after #:phases and #:configure-flags.
>
> Some people like the ‘extreme indentation’ you get by throwing away
> half of your screen width.  I find it leads to cramped code and noisy
> patches once the phases need to get actual work done or an even longer
> CMAKE_ flag comes along.
>
> I also added some newlines and tried to group related flags.
>

Thanks, I didn't know that would make the line fit better on
screen. Much neater=)

>> + "-DCMAKE_C_FLAGS=-fvisibility=hidden"
>> + "-DCMAKE_CXX_FLAGS=-fvisibility=hidden"
>
> Should these be explained in a very brief comment?
>

They are from https://github.com/malaterre/GDCM/blob/master/PACKAGER,
the explanation is:
"This make sure that on UNIX, the API is actually identical at what is found on Windows."

>
> Thank you for building with system libraries!  Also remove the bundled
> copies when possible.  I did so in a (rather strict) source snippet.
>

Ok, neat=)

>> + "-DGDCM_PDF_DOCUMENTATION:BOOL=OFF"
>
> I cannot get the man pages to build, either.  They need something
> called ‘xsl-ns’.  I've disabled GDCM_BUILD_DOCBOOK_MANPAGES for now.
>
>> + "-DGCM_BUILD_TESTING:BOOL=OFF"
>
> Why is this set?  It's reported by CMake as having no effect, and a
> diff of the output confirms that.
>

From the old wiki:

"This boolean is responsible for deciding whether or not to build/run the
nightly regression test of gdcm. Warning when turning this option on,
the size of the gdcm libraries will be bigger since some extra code are
compiled in for the testing framework (see gdcm::Testing, and the md5
lib)."

This seems to be incorrect then, maybe we can skip it.

>> +    (license license:bsd-3)))
>
> I still need to check this.
>

https://github.com/malaterre/GDCM/blob/master/Copyright.txt

I'm not able to apply your new patch, but that is probably a fault on my part.

Thanks a lot for sharing your time and knowledge, and for making this
patch neater! I find this a lot of fun, but have no experience
with scheme or packaging, so your explanations are very valuable to me.

Cheers
Tor-björn

Hi again!

Your patch applies perfectly, the error was on my side.

Also, a gdcm package was added to the bioinformatics module 4 days ago=) It
packages an older version (2.8.9) but is much more concise than my attempt.
Would it be worthwhile to continue work on this package as perhaps gdcm-3.0
and move it to bioinformatics? I put it in image-processing since dcmtk was
already there.

Best regards

Den ons 11 jan. 2023 kl 08:08 skrev Tor-björn Claesson <tclaesson@gmail.com
>:

>
> Hi!
>
> Tobias Geerinckx-Rice <me@tobias.gr> writes:
> >
> >> 3. It does not perform tests.
> >
> > OK, I'll take a look.
> >
> > If tests are disabled, the reason should always be noted in a comment.
> > Even if it's just ‘; no test suite’.
> >
>
> #:tests? #t makes the build fail with "make: *** No rule to make target
>  'test'.  Stop."
>
> GDCM has nightly regression tests
> (https://open.cdash.org/index.php?project=GDCM), should we try to run
> those when building? I have tried to find out how to do this but for now
> with no success. Maybe it is obvious to more experienced people?
>
> >> +(define-public gdcm
> >
> > It used to be common to unconditionally add packages to the end of
> > files, but this needlessly increased the risk of merge conflicts.
> >
> > Instead, just add them wherever they first fit alphabetically; here, I
> > put it above ‘mia’.
> >
>
> Ok, will do from now on!
>
> >> +    (version "3.0.20")
> >
> > ‘guix lint’ says this can be updated to 3.1.0 but I didn't try, as I'd
> > rather it be tested by an actual user — i.e., you.
> >
>
> I got that too, but the latest release in git is 3.0.20
>
> >> + "1w78cmm9q7aavs7svdkl4dgilcqk4yazci9m6x7icrssb7cj991i"))))
> >> +    (build-system cmake-build-system)
> >> +    (outputs '("out" "doc"))
> >
> > /share/doc wasn't actually installed into "doc", but to "out", so I
> > set the GDCM_INSTALL_DOC_DIR configure flag.
> >
> >> +    (arguments
> >> +     (list #:tests? #f
> >> +           #:phases #~(modify-phases %standard-phases
> >> +                        (add-before 'configure 'set-LDFLAGS
> >> +                          (lambda* (#:key inputs outputs
> >> #:allow-other-keys)
> >> +                            (setenv "LDFLAGS"
> >> +                                    (string-append "-Wl,-rpath="
> >> +                                                   #$output
> >> "/lib"))))
> >> +                        (add-before 'build 'patch-gdcm-charls.h
> >> +                          (lambda _
> >> +                            (substitute*
> >> "../source/Utilities/gdcm_charls.h"
> >> +                              (("# include <CharLS/charls.h>")
>
> Ah, good catch!
>
> >
> > Purely as a matter of taste I dropped the ‘# include ’ from both
> > strings and escaped the ‘.’ in the regexp.
> >
> >> +                               "# include <charls/charls.h>"))
> >> #t)))
> >
> > ‘#t’ endings are also obsolete.  Just drop them entirely.  Phases can
> > now safely return anything, including nothing or undefined.
> >
> > I added the following phase to work around log spam, since I didn't
> > find its source (nor did I look very hard) [edit: it was indeed
> > graphviz, thanks].  By default, $HOME is not writable in the build
> > environment.
> >
> >  (add-before 'build 'set-HOME
> >    ;; The build spams ‘Fontconfig error: No writable cache
> >    ;; directories’ in a seemingly endless loop otherwise.
> >    (lambda _
> >      (setenv "HOME" "/tmp")))
> >
> >> +           #:configure-flags #~(list "-DCMAKE_SKIP_RPATH:BOOL=YES"
>
> Is this needed, btw? It came from gdcm:s packaging
> instructions. Removing it causes no verify-runpath issues.
>
> >
> > I, opinionated, added newlines after #:phases and #:configure-flags.
> >
> > Some people like the ‘extreme indentation’ you get by throwing away
> > half of your screen width.  I find it leads to cramped code and noisy
> > patches once the phases need to get actual work done or an even longer
> > CMAKE_ flag comes along.
> >
> > I also added some newlines and tried to group related flags.
> >
>
> Thanks, I didn't know that would make the line fit better on
> screen. Much neater=)
>
> >> + "-DCMAKE_C_FLAGS=-fvisibility=hidden"
> >> + "-DCMAKE_CXX_FLAGS=-fvisibility=hidden"
> >
> > Should these be explained in a very brief comment?
> >
>
> They are from https://github.com/malaterre/GDCM/blob/master/PACKAGER,
> the explanation is:
> "This make sure that on UNIX, the API is actually identical at what is
> found on Windows."
>
> >
> > Thank you for building with system libraries!  Also remove the bundled
> > copies when possible.  I did so in a (rather strict) source snippet.
> >
>
> Ok, neat=)
>
> >> + "-DGDCM_PDF_DOCUMENTATION:BOOL=OFF"
> >
> > I cannot get the man pages to build, either.  They need something
> > called ‘xsl-ns’.  I've disabled GDCM_BUILD_DOCBOOK_MANPAGES for now.
> >
> >> + "-DGCM_BUILD_TESTING:BOOL=OFF"
> >
> > Why is this set?  It's reported by CMake as having no effect, and a
> > diff of the output confirms that.
> >
>
> From the old wiki:
>
> "This boolean is responsible for deciding whether or not to build/run the
> nightly regression test of gdcm. Warning when turning this option on,
> the size of the gdcm libraries will be bigger since some extra code are
> compiled in for the testing framework (see gdcm::Testing, and the md5
> lib)."
>
> This seems to be incorrect then, maybe we can skip it.
>
> >> +    (license license:bsd-3)))
> >
> > I still need to check this.
> >
>
> https://github.com/malaterre/GDCM/blob/master/Copyright.txt
>
> I'm not able to apply your new patch, but that is probably a fault on my
> part.
>
> Thanks a lot for sharing your time and knowledge, and for making this
> patch neater! I find this a lot of fun, but have no experience
> with scheme or packaging, so your explanations are very valuable to me.
>
> Cheers
> Tor-björn
>