dcmtk Document overriding mechanism of DCMTK_DEFAULT

Oct 11 '21 12:10 malaterre

Does it make more sense to make an option out of it then?

Nov 05 '21 16:11 michaelonken

@michaelonken I believe this would make more sense, setting a cache variable is somewhat counter-intuitive (IMHO). I'll try to prepare a patch in that direction (unless you know how to implement it already). I'll close this one when I push the other.

Nov 07 '21 09:11 malaterre

@michaelonken In fact that was just a string replacement. Please review. Thanks

Nov 23 '21 10:11 malaterre

@malaterre If I start a new build from scratch using this patch, CMake reports Info: DCMTK will compile without any default dictionary which is not desired (instead built-in on Windows, external on Unix systems).

Nov 25 '21 11:11 michaelonken

@michaelonken Deeply sorry for the previous patch, this was clearly not carefully tested. See updated patch. Thanks.

Nov 25 '21 13:11 malaterre

Hey Mathieu, I just looked at this patch. I tried with the old mechanism (i.e. before your patch): cmake -DDCMTK_DEFAULT_DICT=builtin , which also does the trick. Also calling afterwards cmake -DDCMTK_DEFAULT_DICT=external changes the setting to the external dictionary.

Which use case is covered by the patch not covered by the behavior above?

Thank you

Mar 22 '22 17:03 michaelonken

@michaelonken In case you've not seen it yet, the commit message states:

Previously DCMTK_DEFAULT_DICT was declared as CACHE variable, which would force users to write:
set(DCMTK_DEFAULT_DICT
    "builtin"
    CACHE STRING "")
add_subdirectory(dcmtk)
Using the new cmake logic, user can simply:
set(DCMTK_DEFAULT_DICT "builtin")
add_subdirectory(dcmtk)

Do you want to enter into more details on how to reproduce ? You cannot reproduce it directly from the dcmtk source tree, you need an extern source tree (eg. ITK) followed by add_subdirectory.

Mar 23 '22 08:03 malaterre

Ok I think I got it -- this is useful for simplifying configuration of DCMTK as an external project.

(The point was that if I directly configure DCMTK from the command line using cmake -DDCMTK_DEFAULT_DICT=... this also works in the current "cache" version without problems, so I did not see why I should change it to an option instead of cache variable)

So if I got you right now, I will take over the patch.

Mar 23 '22 10:03 michaelonken

Hi Mathieu, I just pushed this as 25fa15 to our internal CI build. If it's fine, it will show up soon on the public master branch.

Jan 04 '23 10:01 michaelonken

Document overriding mechanism of DCMTK_DEFAULT_DICT