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

ENH: itktiff: Update source tree layout to match upstream #4957

Merged
merged 9 commits into from
Nov 17, 2024

Conversation

bradking
Copy link
Member

@bradking bradking commented Nov 15, 2024

Re-import tiff 4.0.3 from upstream using our modern history structure.
Retain local edits made by ITK since 4.0.3 was imported previously.

Issue: #4953

bradking and others added 8 commits November 15, 2024 11:22
Remove files not needed since the update to libtiff 4.0.3 by
commit 54c790a (ENH: updating to libtif 4.0.3, 2012-11-09,
v4.3rc01~24^2).
Import version 4.0.3 to match what was last manually imported by commit
54c790a (ENH: updating to libtif 4.0.3, 2012-11-09, v4.3rc01~24^2).
Code extracted from:

    https://gitlab.com/libtiff/libtiff.git

at commit 21a904d74c72b86d7daf9e4d4e6301b6322894bc (v4.0.3).
# By Tiff Upstream
* upstream-tiff:
  tiff 2012-09-22 (21a904d7)
Manufacture a merge commit with the second parent pointing at the commit
before the old import was removed so that `git blame` can follow the
edited lines back through their original history.
@github-actions github-actions bot added the type:Enhancement Improvement of existing methods or implementation label Nov 15, 2024
@bradking
Copy link
Member Author

This needs to be merged to master without any rebasing or squashing.

@hjmjohnson
Copy link
Member

@thewtex Should we move forward even though the ITK.Pixi-Cxx (windows-2022) is failing on all PRs?

@blowekamp
Copy link
Member

I see the following build error:
/MANIFESTFILE:Modules\Core\TestKernel\src\CMakeFiles\itkTestDriver.dir/intermediate.manifest Modules\Core\TestKernel\src\CMakeFiles\itkTestDriver.dir/manifest.res" failed (exit code 1169) with the following output:
itktiff-6.0.lib(tif_luv.c.obj) : error LNK2005: __ucrt_int_to_float already defined in itktiff-6.0.lib(tif_aux.c.obj)

itktiff-6.0.lib(tif_color.c.obj) : error LNK2005: __ucrt_int_to_float already defined in itktiff-6.0.lib(tif_aux.c.obj)

@bradking
Copy link
Member Author

@blowekamp I'm not able to reproduce that error on Windows, and CI is happy too. Can you reproduce it in a fresh build tree?

@blowekamp
Copy link
Member

@thewtex Should we move forward even though the ITK.Pixi-Cxx (windows-2022) is failing on all PRs?

@thewtex Are the only configuration setting on the Pixi build here: https://github.com/InsightSoftwareConsortium/ITK/blob/master/pyproject.toml#L26-L34

I don't see the job published to CDash, but the configuration is here:
https://github.com/InsightSoftwareConsortium/ITK/actions/runs/11862520836/job/33062081900

I don't see anything particularly different about this build that might be causing this error.

@thewtex
Copy link
Member

thewtex commented Nov 17, 2024

@thewtex Should we move forward even though the ITK.Pixi-Cxx (windows-2022) is failing on all PRs?

It's not failing on all PR's. It has mostly been successfully passing:

https://github.com/InsightSoftwareConsortium/ITK/actions/workflows/pixi.yml?query=branch%3Amaster

It passed on this recent PR:

https://github.com/InsightSoftwareConsortium/ITK/actions/runs/11860511746/job/33055924587?pr=4956

@thewtex
Copy link
Member

thewtex commented Nov 17, 2024

Are the only configuration setting on the Pixi build here: https://github.com/InsightSoftwareConsortium/ITK/blob/master/pyproject.toml#L26-L34

These are the ITK configuration settings. Also worth noting is that it is using the conda-forge cxx-compiler package, version 1.8.0.

@thewtex
Copy link
Member

thewtex commented Nov 17, 2024

@blowekamp I'm not able to reproduce that error on Windows, and CI is happy too. Can you reproduce it in a fresh build tree?

It's failing in CI, with fresh builds.

@thewtex
Copy link
Member

thewtex commented Nov 17, 2024

A few Git bisects may helpful to find the underlying issue.

@bradking
Copy link
Member Author

The __ucrt_int_to_float already defined in itktiff error in question predates this PR, and is #4820.

Extend the condition from commit 08bf6be (COMP: Conditionalize
insane "#define inline" in libtiff, 2013-10-07, v4.5rc01~139^2).
Newer MSVC versions need it to avoid duplicating `__ucrt_int_to_float`.

Issue: InsightSoftwareConsortium#4820
@blowekamp
Copy link
Member

Nice Job tracking that down!

@blowekamp blowekamp merged commit 9dc3985 into InsightSoftwareConsortium:master Nov 17, 2024
16 of 17 checks passed
@bradking bradking deleted the update-tiff branch November 18, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Enhancement Improvement of existing methods or implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants