Skip to content

ENH: Test Linux ARM in CI#5137

Open
thewtex wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
thewtex:linux-arm-ci
Open

ENH: Test Linux ARM in CI#5137
thewtex wants to merge 2 commits intoInsightSoftwareConsortium:mainfrom
thewtex:linux-arm-ci

Conversation

@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 labels Jan 17, 2025
@thewtex thewtex requested a review from Copilot January 17, 2025 20:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Copy link
Member

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

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

@thewtex The code looks good to me. I rebased it the current upstream/main branch

@thewtex
Copy link
Member Author

thewtex commented Jan 29, 2026

@hjmjohnson thanks, but the workflow is invalid:

https://github.com/InsightSoftwareConsortium/ITK/actions/runs/21444681186/workflow

And there are also test failures and warnings that need to be addressed.

@hjmjohnson hjmjohnson force-pushed the linux-arm-ci branch 3 times, most recently from b9bb4d6 to d77f751 Compare February 2, 2026 13:58
@hjmjohnson
Copy link
Member

Build occurs O.K. but the following tests have failures:

itkMINCImageIOTest-COM-t1_z+_byte_trans-1
itkMINCImageIOTest-COM-t1_z-_byte_trans-1
itkMINCImageIOTest-COM-t1_z+_byte_sag-1 
itkMINCImageIOTest-COM-t1_z-_byte_sag-1 
itkMINCImageIOTest-COM-t1_z+_byte_cor-1 
itkMINCImageIOTest-COM-t1_z-_byte_cor-1 
itkMINCImageIOTest-COM-t1_z+_byte_trans-0
itkMINCImageIOTest-COM-t1_z-_byte_trans-0
itkMINCImageIOTest-COM-t1_z+_byte_sag-0 
itkMINCImageIOTest-COM-t1_z-_byte_sag-0 
itkMINCImageIOTest-COM-t1_z+_byte_cor-0 
itkMINCImageIOTest-COM-t1_z-_byte_cor-0 
itkMINCImageIOTest3     

itkHDF5ImageIOTest 

I think the CI component is working as expected, but some code changes are needed to build on Linux ARM environments.

@dzenanz dzenanz requested review from gdevenyi and vfonov February 3, 2026 14:31
@github-actions github-actions bot added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:IO Issues affecting the IO module labels Feb 3, 2026
@hjmjohnson
Copy link
Member

FYI: I think the crux of the problem lies in how MetaData is handled in ITK for HDF5 image IO.

Failure ExposeMetaData 'TestBool'

Failure ExposeMetaData 'TestUChar'
Incorrect meta value read in for TestUChar '
' != 'u'

@hjmjohnson hjmjohnson marked this pull request as draft February 3, 2026 20:58
@hjmjohnson
Copy link
Member

The meta data components of the hd5 file written to disk are different on M3 mac for mac build vs docker linxu-arm build:

h5dump ./Testing/Temporary/ITKIOHDF5/RGBImage.hdf5 ~/PASS_MAC
h5dump ./Testing/Temporary/ITKIOHDF5/RGBImage.hdf5 > ~/FAIL_LINUX
vimdiff ~/PASS_MAC ~/FAIL_LINUX
image

@github-actions github-actions bot added area:Bridge Issues affecting the Bridge module area:Filtering Issues affecting the Filtering module area:Registration Issues affecting the Registration module area:ThirdParty Issues affecting the ThirdParty module labels Feb 9, 2026
@hjmjohnson
Copy link
Member

hjmjohnson commented Feb 12, 2026

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hjmjohnson
Copy link
Member

@N-Dekker @thewtex @blowekamp @dzenanz

I think this is finally ready for review!

I tested this on Arm-mac with

  • ITK_FUTURE_REMOVE=ON, compiler flags char->signed
  • ITK_FUTURE_REMOVE=ON, compiler flags char->unsigned
  • ITK_FUTURE_REMOVE=OFF, compiler flags char->signed
  • ITK_FUTURE_REMOVE=OFF, compiler flags char->unsigned
  • linux-arm docker on arm-mac image

I used h5dump on the created .hdf files to verify that the metadata data types were preserved in all but one case.

PREVIOUSLY : If an .h5 image file was written with a metadata element of type 'char', it was written in the .h5 file as either explicitly unsigned or signed (see image in previous comment). If it were attempted to be read by a binary with a different signedness of 'char', it would not match the requested type and would fail to read the meta-data.

The current code retains backward compatibility if old h5 files with 'char' based meta data as long as the only ASCII characters (i.e. values between 0-127) were used (all the test cases), OR if the the files were written from platforms where the 'char' is a signed data type.

In other words, only files that were written from a platform where 'char' was unsigned AND contained values larger than 127 will be interpreted differently, but that is impossible to determine if from the file.

thewtex and others added 2 commits February 12, 2026 08:34
Initial resolution for addressing ambiguitiy in 'char' signed behavior desicribed
in issue Related to: InsightSoftwareConsortium#5779

`VerifyMetaData` function to eliminate redundant metadata handling code in
`HDF5ImageIOTest` and `MetaDataDictionaryGTest`. This
simplifies test logic and improves maintainability.
Reduce duplication of common testing.

Provide more failure diagnostics to assist with pin-pointing
the failure locations.

Replace `dynamic_cast` with `static_cast` and explicit type information
checks for clearer and more efficient metadata handling.

On linux-arm <char> and <unsigned char> are the same types.
On most other platforms, <char> is the same as <signed char>.
@github-actions github-actions bot removed area:Bridge Issues affecting the Bridge module area:Filtering Issues affecting the Filtering module area:Registration Issues affecting the Registration module labels Feb 12, 2026
/* Used only to avoid duplicate code in itkMetaDictionaryGTest.cxx and itkHDF5ImageIOTest.cxx */
template <typename T>
int
VerifyMetaDataPrivateTestingUtility(const itk::MetaDataDictionary & metaDict,
Copy link
Member

Choose a reason for hiding this comment

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

This function should go in the TestKernel module if it's used for testing in multiple modules.


template <typename T>
static int
CheckMetaData(itk::MetaDataDictionary & metaDict, const std::string & key, const T & knownValue)
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding as a GTestPredicate in TestKernel/../itkGTestPredicate.h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:IO Issues affecting the IO module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants