Skip to content

WIP: Manual update to expat 2.7.1 #5488

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
Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

WIP: Manual update to expat 2.7.1 #5488

wants to merge 8 commits into from

Conversation

hjmjohnson
Copy link
Member

ENH: Organized files to match upstream expat directory layout in preparation
for minimizing differences.

Instrument with comments to clearly identify where differences from
upstream are desired in the CMakeLists.txt configurations.

Followup from 38cca37 and requests
for updates in #5351.

PR Checklist

@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 labels Aug 19, 2025
dzenanz
dzenanz reviewed Aug 19, 2025
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.

Mostly looks good after a short look.

@hjmjohnson
ENH: Manual update to expat 2.7.1
59f654e 
Organized files to match upstream expat directory layout in preparation
for minimizing differences.

Instrument with comments to clearly identify where differences from
upstream are desired in the CMakeLists.txt configurations.

Followup from 38cca37 and requests
for updates in #5351.
@hjmjohnson
ENH: Sort defines in alphabetical order to minimize differences.
eac6c15
@hjmjohnson
ENH: Adding new symbol for expat 2.7.1
47e7466
@hjmjohnson
ENH: Update Expat headers to streamline itk_expat_mangle.h inclusion
c0345c9 
The name mangling header should be included directly rather than
as a side-effect of including expatDllConfig.h
@hjmjohnson
ENH: Remove unsued header file that is no longer used.
57072b0
@hjmjohnson
ENH: Add explicit error for missing itk_expat_mangle.h in Expat sources
7793d23 
Inserted checks in Expat source files to ensure proper inclusion
of itk_expat_mangle.h, preventing incorrect header inclusion leading to
expat library name mangling failures.
@hjmjohnson
ENH: Remove unused expatDllConfig.h references and associated file
086e7a1 
This file, and the contents of this file (the definition of ITK_EXPAT_STATIC) are not
used anywhere in ITK.
dzenanz
dzenanz reviewed Aug 20, 2025
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.

Mostly looks good. It would be good if @agravgaard, @npt-1707, and @SimonRit reviewed this too.

@dzenanz dzenanz requested a review from SimonRit August 20, 2025 18:40
SimonRit
SimonRit suggested changes Aug 21, 2025
View reviewed changes
Copy link

@SimonRit SimonRit left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I find it hard to review third party upgrades but as RTK uses expat, I have tested this branch and it seems to fail:
https://github.com/RTKConsortium/RTK/actions/runs/17121664677/job/48563766375
Can you try to fix these unresolved external symbol errors?
It would be good if @agravgaard could review indeed but I'm not sure he's still in medical imaging (would be good to know!).

@hjmjohnson
Copy link
Member Author

@SimonRit Thanks. I am trying to gain access to a windows build computer so that I can identify why the windows name mangling is not working. This is also failing in a similar way for windows builds on the main ITK repository.

Hans

@agravgaard
Copy link
Contributor

Thanks for the tag, but I'm indeed not in medical imaging anymore.
Looks fine to me, but I can't easily test it currently.
(I'll try to give it a spin, but I suspect my old build chain has broken with time, so don't wait for it)

@hjmjohnson
COMP: EXPAT_SHARED_LIBS must match BUILD_SHARED_LIBS
938d4ad 
The upstream expat build has a complicated relationship
between BUILD_SHARED_LIBS and EXPAT_SHARED_LIBS that
can cause the two variables to be out of sync.

ITK's internal use of expat requires that EXPAT_SHARED_LIBS
have the same value as BUILD_SHARED_LIBS.
@hjmjohnson
Copy link
Member Author

This is much more challenging that expected. I need to review the upstream code to determine if it needs to be fixed.

@hjmjohnson hjmjohnson marked this pull request as draft August 22, 2025 18:24
@hjmjohnson hjmjohnson changed the title ENH: Manual update to expat 2.7.1 WIP: Manual update to expat 2.7.1 Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz approved these changes

+1 more reviewer

@SimonRit SimonRit SimonRit requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area:ThirdParty Issues affecting the ThirdParty module type:Enhancement Improvement of existing methods or implementation 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.
4 participants
@hjmjohnson @agravgaard @SimonRit @dzenanz