Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENH: Update mangled third-party libraries to use MANGLE_PREFIX CMake variable #4102

Merged
merged 1 commit into from
Jul 12, 2023
Merged

ENH: Update mangled third-party libraries to use MANGLE_PREFIX CMake variable #4102

merged 1 commit into from
Jul 12, 2023

Conversation

jcfr
Copy link
Contributor

@jcfr jcfr commented Jul 8, 2023 •
edited
Loading

This commit updates mangled third-party libraries to consistently configure the mangle header based on the value of the MANGLE_PREFIX CMake variable.

This pull request is done in the context of the topic Adding support for customizing ITK namespace. See https://discourse.itk.org/t/adding-support-for-customizing-itk-namespace/5170/2

Third-party Modules

Updated from mangle.h to _mangle.h.in:

  • Expat
  • HDF5
  • JPEG
  • NrrdIO
  • PNG
  • TIFF
  • VNL (ThirdParty/VNL/src/vxl/v3p/netlib/(v3p_f2c_mangle.h|v3p_netlib_mangle.h))

Already using _mangle.h.in:

  • GDCM (gdcmmd5, gdcmuuid, gdcmopenjpeg)
  • OpenJPEG

Not yet using mangling

These may be updated in follow-up pull-requests

  • DCMTK
  • DoubleConversion (libitkdouble-conversion)
  • Eigen3 (mangling not needed - header only library)
  • GDMC (remaining libraries)
  • GIFTI
  • libLBFGS
  • MetaIO
  • MINC (libitkminc2)
  • Netlib (mangling not needed - statically built)
  • pygccxml (mangling not needed - used for wrapping)
  • TBB
  • VNL (libitkvcl, libitkvnl, libitkvnl_algo)
  • VNLInstantiation (mangling not needed - statically built)
  • ZLIB

Related pull requests

PR Checklist

@jcfr
ENH: Update mangled 3rd-parties to use MANGLE_PREFIX CMake variable
a3afcf6 
This commit updates mangled libraries to consistently configure
the mangle header based on the value of the MANGLE_PREFIX CMake variable.

It updates the following libraries:
* Expat
* HDF5
* JPEG
* NrrdIO
* PNG
* TIFF
@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation area:ThirdParty Issues affecting the ThirdParty module type:Data Changes to testing data labels Jul 8, 2023
@jcfr jcfr removed type:Enhancement Improvement of existing methods or implementation type:Data Changes to testing data labels Jul 8, 2023
dzenanz
dzenanz reviewed Jul 10, 2023
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zlib has a prefix. It is set here:
https://github.com/InsightSoftwareConsortium/ITK/blob/v5.4rc01/Modules/ThirdParty/ZLIB/src/CMakeLists.txt#L5

Have you tried integrating this into Slicer, and does it work?

Modules/ThirdParty/Expat/src/expat/CMakeLists.txt
@@ -178,6 +178,11 @@ CONFIGURE_FILE(${ITK3P_EXPAT_SOURCE_DIR}/expatDllConfig.h.in

configure_file(expat_config.h.cmake "${ITK3P_EXPAT_BINARY_DIR}/expat_config.h")

set(MANGLE_PREFIX itk_expat)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this respect cache? If the calling code has set(MANGLE_PREFIX slicer_expat), will this line not override that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are local variables not intended to allow customization.

Customization of mangling will be done in a follow-up pull request introducing code like:

set(MANGLE_PREFIX ${ITK_NAMESPACE}_expat)

or

set(MANGLE_PREFIX ${ITK_NAMESPACE})

based on the third-party library mangling requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purpose of these changes it to introduce a uniform mangling interface for all third-parties.

@thewtex thewtex merged commit 93ed7dd into InsightSoftwareConsortium:master Jul 12, 2023
@jcfr jcfr mentioned this pull request Oct 30, 2023
Merged
@dzenanz dzenanz mentioned this pull request Nov 8, 2024
Merged
7 tasks
@dzenanz dzenanz mentioned this pull request Nov 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz left review comments

@thewtex thewtex thewtex approved these changes

Assignees
No one assigned
Labels
area:ThirdParty Issues affecting the ThirdParty module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jcfr @thewtex @dzenanz