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

Itk mac support #10212

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Itk mac support #10212

wants to merge 8 commits into from

Conversation

divital-coder
Copy link
Contributor

Added macOs compilation support for libITK

giordano
giordano reviewed Jan 7, 2025
View reviewed changes
I/ITK/build_tarballs.jl
Dependency(PackageSpec(name="JpegTurbo_jll", uuid="aacddb02-875f-59d6-b918-886e6ef4fbf8")),
Dependency(PackageSpec(name="Libtiff_jll", uuid="89763e89-9b03-5906-acba-b20f662cd828")),
Dependency(PackageSpec(name="libpng_jll", uuid="b53b4c65-9356-5827-b1ea-8c7a1a84506f")),
Dependency(PackageSpec(name="Eigen_jll", uuid="bc6bbf8a-a594-5541-9c57-10b0d0312c70")),
Copy link
Member

Choose a reason for hiding this comment

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

Isn't eigen header only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, the binarybuilder wizard errors, if i do not include all the dependency packages, so followed the same routine.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying you should remove it, but if eigen isn't needed at runtime then it should be a build-only dependency: https://docs.binarybuilder.org/stable/reference/#BinaryBuilderBase.BuildDependency. So the question is: is the eigen package really needed at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And indeed it is header only,
https://stackoverflow.com/questions/51887137/eigen-cmake-requires-target-link-libraries-even-though-its-template-only/51887180#51887180

giordano
giordano requested changes Jan 7, 2025
View reviewed changes
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

Since this build will change compat bounds, you must change the version number. Use 5.3.1 (doesn't look like upstream will use that number)

@divital-coder
Copy link
Contributor Author

Very Strange, the build succeeds on my x86_64 linux, with glibc

@divital-coder
Copy link
Contributor Author

Since this build will change compat bounds, you must change the version number. Use 5.3.1 (doesn't look like upstream will use that number)

Bumping the minor version is causing the package build fail on most of the platforms, primarily due to one of the modules in the ITK package, Specifically following is the issue :

[09:18:46] In file included from /workspace/srcdir/ITK/build/Modules/Core/QuadEdgeMesh/test/ITKQuadEdgeMeshHeaderTest1.cxx:23:
[09:18:46] /workspace/srcdir/ITK/Modules/Core/QuadEdgeMesh/include/itkQuadEdgeMeshLineCell.h:121:3: error: ‘CellGeometryEnum’ does not name a type
[09:18:46]    CellGeometryEnum
[09:18:46]    ^~~~~~~~~~~~~~~~
[09:18:46] /workspace/srcdir/ITK/Modules/Core/QuadEdgeMesh/include/itkQuadEdgeMeshLineCell.h:125:20: error: ‘CellGeometryEnum’ does not name a type
[09:18:46]    static constexpr CellGeometryEnum
[09:18:46]                     ^~~~~~~~~~~~~~~~
[09:18:46] In file included from /workspace/srcdir/ITK/Modules/Core/QuadEdgeMesh/include/itkQuadEdgeMeshLineCell.h:242,
[09:18:46]                  from /workspace/srcdir/ITK/build/Modules/Core/QuadEdgeMesh/test/ITKQuadEdgeMeshHeaderTest1.cxx:23:
[09:18:46] /workspace/srcdir/ITK/Modules/Core/QuadEdgeMesh/include/itkQuadEdgeMeshLineCell.hxx:272:1: error: no declaration matches ‘itk::CellGeometryEnum itk::QuadEdgeMeshLineCell<TCellInterface>::GetType() const’
[09:18:46]  QuadEdgeMeshLineCell<TCellInterface>::GetType() const
[09:18:46]  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[09:18:46] /workspace/srcdir/ITK/Modules/Core/QuadEdgeMesh/include/itkQuadEdgeMeshLineCell.hxx:272:1: note: no functions named ‘itk::CellGeometryEnum itk::QuadEdgeMeshLineCell<TCellInterface>::GetType() const’
[09:18:46] In file included from /workspace/srcdir/ITK/build/Modules/Core/QuadEdgeMesh/test/ITKQuadEdgeMeshHeaderTest1.cxx:23:
[09:18:46] /workspace/srcdir/ITK/Modules/Core/QuadEdgeMesh/include/itkQuadEdgeMeshLineCell.h:39:27: note: ‘class itk::QuadEdgeMeshLineCell<TCellInterface>’ defined here
[09:18:46]  class ITK_TEMPLATE_EXPORT QuadEdgeMeshLineCell
[09:18:46]                            ^~~~~~~~~~~~~~~~~~~~
[09:18:46] make[2]: *** [Modules/Core/QuadEdgeMesh/CMakeFiles/ITKQuadEdgeMeshHeaderTest1.dir/build.make:80: Modules/Core/QuadEdgeMesh/CMakeFiles/ITKQuadEdgeMeshHeaderTest1.dir/test/ITKQuadEdgeMeshHeaderTest1.cxx.o] Error 1
[09:18:46] make[2]: Leaving directory '/workspace/srcdir/ITK/build'
[09:18:46] make[1]: *** [CMakeFiles/Makefile2:12113: Modules/Core/QuadEdgeMesh/CMakeFiles/ITKQuadEdgeMeshHeaderTest1.dir/all] Error 2

@divital-coder divital-coder requested a review from giordano January 7, 2025 11:34
@giordano
Copy link
Member

giordano commented Jan 7, 2025

Bumping the minor version is causing the package build fail on most of the platforms

How's the version number possibly related to anything happening inside the build environment as that variable isn't used anywhere? Also, you must change the version number, I'm not going to merge the PR without that, and even if I did, it'd fail to be registered in General, reason why I'm telling you to change the version number.

@divital-coder
Copy link
Contributor Author

I am just as confused, thats just an arbitrary variable, and i know that changing version is required, but its just buildkite is failing builds, let me try again with that version.

@divital-coder
Copy link
Contributor Author

divital-coder commented Jan 7, 2025 •
edited
Loading

@giordano See, in the last commit
this one

[[ITK-v5.3.] fixed libitkdouble-conversion-5.3 library product naming](https://github.com/JuliaPackaging/Yggdrasil/pull/10212/commits/0d5d6189a0f2ce654ffc265cc8980c07f048e837)

u can see it was successful but this commit which is literally about a version string, is causing issues, somewhat this version change is related then i guess. Should i discard platforms for which the build is failing, i surely have reported the error to the ITK forums, a patch from their side perhaps is somewhat needed for some platforms for ITK-5.3.0

@divital-coder
Copy link
Contributor Author

What is going on???? It literally appears that the any chosen first build is randomly failing.

@giordano
Copy link
Member

giordano commented Jan 7, 2025

I don't have the time to look at this today (and probably for most of the rest of the week)

@divital-coder
Copy link
Contributor Author

no worries, not in a hurry, have an awesome week on your side of the screen :)

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

@giordano giordano Awaiting requested review from giordano

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@divital-coder @giordano