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.

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

libtiff: update to 4.7.0 #4961

Merged
merged 5 commits into from
Nov 20, 2024

Conversation

blowekamp
Copy link
Member

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

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.

Generally looks good. Do you plan to do an update to 4.7.0 now, or is that left for later?

Modules/ThirdParty/TIFF/UpdateFromUpstream.sh Outdated Show resolved Hide resolved
@blowekamp
Copy link
Member Author

Generally looks good. Do you plan to do an update to 4.7.0 now, or is that left for later?

I think it will be done in this PR.

VTK's 4.6.0 changes were used and were committed in the same sub-tree as upstream. However this changes get clobbered when the updating to 4.7.0. I believe that the merge of the VTK changes need to be "flattened" so that they are in the main ITK tree.

@bradking How do you suggest fixing the history so that updating to 4.7.0 will not remove the VTK changes?

@bradking
Copy link
Member

I don't think we should import into ITK from VTK's third-party repo. I'll rebuild this to make the net changes directly.

@blowekamp
Copy link
Member Author

blowekamp commented Nov 20, 2024

I don't think we should import into ITK from VTK's third-party repo. I'll rebuild this to make the net changes directly.

I did a "git cherry-pick -m 1 7408285" and that seemed to "flatten the merge into a single parent commit.

EDIT: Just updated the PR with that WIP.

@bradking
Copy link
Member

@blowekamp I rebuilt your PR to update directly from upstream tiff 4.7.0 and apply ITK-specific build system edits.

@bradking bradking changed the title Update libtiff libtiff: update to 4.7.0 Nov 20, 2024
@bradking
Copy link
Member

@blowekamp if there are any problems in CI, you can just edit the last commit or add new commits.

This needs to be merged with a real merge commit and no rebase or squash. Then future updates with Modules/ThirdParty/TIFF/UpdateFromUpstream.sh should merge upstream changes into our local edits.

@blowekamp
Copy link
Member Author

@blowekamp if there are any problems in CI, you can just edit the last commit or add new commits.

This needs to be merged with a real merge commit and no rebase or squash. Then future updates with Modules/ThirdParty/TIFF/UpdateFromUpstream.sh should merge upstream changes into our local edits.

Amazing work!

How was the name mangling generated? Does it still need to be run on other systems?

Also It looks like libtiff.def needs to be converted to a "cmake.in" to support @MANGLE_PREFIX@?

Also the libtiff.map file has the unmagled names but it looks like the file is not used and can be removed.

@bradking
Copy link
Member

itk_tiff_mangle.h.in has comments at the top (left from long ago) that describe how to generate the mangling. We don't actually replace @MANGLE_PREFIX@ with anything besides itk_tiff, but that pattern is used throughout ITK's third-party library mangling so I kept it. For libtiff.def I think we can leave it hard-coded.

I'll rebuild the branch with libtiff.map not imported, but we might as well let CI finish first to see if there are any other problems.

@blowekamp
Copy link
Member Author

itk_tiff_mangle.h.in has comments at the top (left from long ago) that describe how to generate the mangling.

Some have been adamant that these command must be run on all platforms to ensure all symbols have been identified. So what platform(s) was it run on?

We don't actually replace @MANGLE_PREFIX@ with anything besides itk_tiff, but that pattern is used throughout ITK's third-party library mangling so I kept it. For libtiff.def I think we can leave it hard-coded.

This is the commit that introduced it:
a3afcf6

My understanding is that 3D Slicer has multiple version of ITK in the runtime. ( ITK Python for pip and local build?) and there were problems with conflicting symbols. Additionally, missing mangled symbols with third partly libraries have caused problems.

@bradking
Copy link
Member

Okay, I pushed an update to drop the .map file and convert the .def file to use @MANGLE_PREFIX@.

The mangling currently accounts for Windows, macOS, and Linux.

@seanm
Copy link
Contributor

seanm commented Nov 20, 2024

Some have been adamant that these command must be run on all platforms to ensure all symbols have been identified.

I've seen this in the past, with HDF5 I think, which both VTK and ITK use, where the mangling was done only with some OSes and some symbols where missed, and then both VTK and ITK have the same symbol and you get a link error.

95% of symbols are of course shared between OSes, but that 5% has bitten us in the past.

@blowekamp blowekamp marked this pull request as ready for review November 20, 2024 18:54
@blowekamp
Copy link
Member Author

@bradking Thanks for all the work!

I checked the symbols an my Mac ARM and nothing were missing. I did have to modify the commands to make the symbols unique and sort them with case insensitivity ( but there were still order difference). I also build from an installed version of ITK successfully too.

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.

@bradking @blowekamp THANK YOU FOR YOUR HARD WORK!!!

Hans

@hjmjohnson hjmjohnson merged commit 7e3492b into InsightSoftwareConsortium:master Nov 20, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants