all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: "Tor-björn Claesson" <tclaesson@gmail.com>
To: Tobias Geerinckx-Rice <me@tobias.gr>
Cc: 60640@debbugs.gnu.org
Subject: [bug#60640] Gnu: Add gdcm
Date: Thu, 12 Jan 2023 13:21:34 +0200	[thread overview]
Message-ID: <CAO0k701qok-m07zB-s0OO1W6JJWYoDc5OF8vmE3_jAe=Z97zSA@mail.gmail.com> (raw)
In-Reply-To: <87o7r5li2j.fsf@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 6073 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 8003 bytes --]

  reply	other threads:[~2023-01-12 11:24 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-07 19:40 [bug#60640] Gnu: Add gdcm Tor-björn Claesson
     [not found] ` <handler.60640.B.167314095112989.ack@debbugs.gnu.org>
2023-01-08  7:59   ` [bug#60640] Acknowledgement (Gnu: Add gdcm) Tor-björn Claesson
2023-01-08  8:04     ` Tor-björn Claesson
2023-01-09  8:28       ` Tobias Geerinckx-Rice via Guix-patches via
2023-01-09 10:50         ` Tor-björn Claesson
2023-01-10  7:01           ` ( via Guix-patches via
2023-01-10 14:40     ` Tobias Geerinckx-Rice via Guix-patches via
2023-01-10 15:10       ` Tor-björn Claesson
2023-01-12 22:17     ` [bug#60640] Gnu: Add gdcm Ludovic Courtès
2023-01-13 10:45       ` Tor-björn Claesson
2023-01-13 23:50         ` Ludovic Courtès
2023-01-14  8:19           ` Tor-björn Claesson
2023-01-14  9:30           ` Tor-björn Claesson
2023-01-17 14:36             ` Ludovic Courtès
2023-01-14 17:06           ` Tor-björn Claesson
2023-01-17 14:38             ` Ludovic Courtès
2023-01-17 19:21               ` Tor-björn Claesson
2023-01-25 22:22                 ` Ludovic Courtès
2023-01-20 11:30               ` Tor-björn Claesson
2023-01-20 12:44                 ` Tor-björn Claesson
2023-01-25 22:01                   ` Ludovic Courtès
2023-01-31 21:42                     ` Tor-björn Claesson
2023-01-25 22:49                   ` Ludovic Courtès
2023-01-27 15:06                     ` Tor-björn Claesson
2023-01-14 19:34           ` Tor-björn Claesson
2023-01-14 21:58           ` Tor-björn Claesson
2023-01-10 14:52 ` Tobias Geerinckx-Rice via Guix-patches via
2023-01-11  5:29   ` Tor-björn Claesson
2023-01-12 11:21     ` Tor-björn Claesson [this message]
2024-02-19 22:41 ` bug#60640: " Sharlatan Hellseher

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAO0k701qok-m07zB-s0OO1W6JJWYoDc5OF8vmE3_jAe=Z97zSA@mail.gmail.com' \
    --to=tclaesson@gmail.com \
    --cc=60640@debbugs.gnu.org \
    --cc=me@tobias.gr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.