Fix DCMTK_ENABLE_STL=True not handled correctly

[tsondergaard]

Don't explicitly test for ON. Rely on standard CMake truthy (1, ON, YES, TRUE, Y, or a non-zero number) and falsy (0, OFF, NO, FALSE, N, IGNORE, NOTFOUND, the empty string, or ends in the suffix -NOTFOUND).

[jriesmeier]

Thank you for your report.

As far as I can see, DCMTK_ENABLE_STL is a CMake option that can either be ON or OFF. Also, your proposed patch changes the way the CMake strings DCMTK_ENABLE_STL_LIST, DCMTK_ENABLE_STL_VECTOR, etc. are evaluated. As far as I can see, the explicit check on "ON" is on purpose (see INSTALL file for details).

Or did I miss anything?

[tsondergaard]

DCMTK has lots of other configure options. DCMTK_ENABLE_STL_${FEATURE} is the only one that is tested explicitly with STREQUAL "ON", the rest are tested in the same way that is done in this PR. Some examples:

[ts@spark dcmtk]$ git grep if.*DCMTK_WITH_THREADS
CMake/GenerateDCMTKConfigure.cmake:if(DCMTK_WITH_THREADS)
[ts@spark dcmtk]$ git grep if.*DCMTK_ENABLE_BUILTIN_DICTIONARY
CMake/GenerateDCMTKConfigure.cmake:if(DCMTK_ENABLE_BUILTIN_DICTIONARY)
CMake/dcmtkPrepare.cmake:if (NOT DCMTK_ENABLE_EXTERNAL_DICTIONARY AND NOT DCMTK_ENABLE_BUILTIN_DICTIONARY)

It is true that "ON" and "OFF" are the traditional values passed for options but other truthy and falsy values are legal too. And the explicit check for STREQUAL "ON" is making the options DCMTK_ENABLE_STL_${FEATURE} unusual which has tripped up at least the conan-center-index dcmtk recipe.

[jriesmeier]

I am not a CMake expert and also not the DCMTK developer who introduced these DCMTK_ENABLE_STL_<FEATURE> "options", but in fact these CMake variables are strings and no options. This is because they can have three different values: ON, OFF and INFERRED:

macro(DCMTK_INFERABLE_OPTION OPTION DESCRIPTION)
  set("${OPTION}" INFERRED CACHE STRING "${DESCRIPTION}")
  set_property(CACHE "${OPTION}" PROPERTY STRINGS "INFERRED" "ON" "OFF")
  # currently, all inferable options are advanced options
  mark_as_advanced("${OPTION}")
endmacro()

[tsondergaard]

I overlooked the INFERRED bit, but my change is still correct. For DCMTK_ENABLE_STL_${FEATURE} the value INFERRED means that the value will be set to the value of DCMTK_ENABLE_STL here: https://github.com/DCMTK/dcmtk/blob/8399564aebcbf65e896d02df1ebf8dc10488bb05/CMake/GenerateDCMTKConfigure.cmake#L1514

After that statement DCMTK_ENABLE_STL_${FEATURE} should be treated like a normal cmake boolean flag which means following the truthy/falsy behaviour specified in https://cmake.org/cmake/help/latest/command/if.html:

if(<variable>) True if given a variable that is defined to a value that is not a false constant

I will update the commit comment to make it more clear.

[jriesmeier]

@offis-jas Could you please check this pull request (and this conversation)? Thank you.

[tsondergaard]

Note that DCMTK_ENABLE_CXX11 is also an inferable variable, but it is not handled by the same mechanism as the DCMTK_ENABLE_STL_${FEATURE}variables. Before this pull request passing -DDCMTK_ENABLE_CXX11=TRUE -DDCMTK_ENABLE_STL=TRUE to cmake turns c++11 on but leaves the stl features off. That's the surprising behaviour I would like to fix.

[offis-jas]

Well, I can't say I remember the reasons for writing it this way at that time. Perhaps it was due to the restriction given for those types of variable that Jörg mentioned. In any case, these restrictions only apply for the CMake GUI, therefore, I lean towards agreeing that we should apply this patch. At least I don't see any harm caused after looking at the code for a few minutes in detail.

On Fri, 10 Jun 2022 12:46:19 -0700 Thomas Sondergaard @.***> wrote:

Note that DCMTK_ENABLE_CXX11 is also an inferable variable, but it is not handled by the same mechanism as the DCMTK_ENABLE_STL_${FEATURE}variables. Before this pull request passing -DDCMTK_ENABLE_CXX11=TRUE -DDCMTK_ENABLE_STL=TRUE to cmake turns c++11 on but leaves the stl features off. That's the surprising behaviour I would like to fix.

-- M.Sc. Jan Schlamelcher Automatisierungs- und Integrationstechnik/Wissenschaftlicher Mitarbeiter | Automation and Integration Technology Group/Research Associate

OFFIS e.V. - Institut für Informatik OFFIS DICOM Team Escherweg 2 - 26121 Oldenburg - Germany Phone/Fax: +49 441 9722-223/111 E-Mail: @.*** URL: https://www.offis.de/

Registergericht: Amtsgericht Oldenburg VR 1956 Vorstand: Prof. Dr. Sebastian Lehnhoff (Vorsitzender), Prof. Dr. techn. Susanne Boll-Westermann, Prof. Dr.-Ing. Axel Hahn, Prof. Dr.-Ing. Andreas Hein, Prof. Dr.-Ing. Wolfgang H. Nebel

Unsere Hinweise zum Datenschutz sind abrufbar unter: https://www.offis.de/datentransparenz.html

Our Data Protection Policy can be found under: https://www.offis.de/en/data-transparency.html

-> Nächste DICOM-Schulung vom 09. bis 10. Mai 2022 (online oder in Präsenz) -> Schulung "Patientenakten mit IHE" vom 11. bis 12. Mai 2022 (online oder in Präsenz) -> Nächste HL7-Schulung vom 23. bis 24. Mai 2022 (online oder in Präsenz) -> Next DICOM courses in English 25 to 28 April 2022, online

[tsondergaard]

@jriesmeier, it sounds like @offis-jas agrees with the change. What is the next move?

[jriesmeier]

@tsondergaard Sorry for the late reply. It's now up to @offis-jas to test the proposed changes in detail and commit them to the "testing branch" (if this makes sense).

[tsondergaard]

@jriesmeier and @offis-jas, okay. I worked around the issue in conan-center-index as you can see in the linked commits above.

[michaelonken]

I tried this and checked this in as commit 087925 anyway, showing up on master within the next days.