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

[cmake] Fixes to build on Silicon #1241

Merged
merged 9 commits into from
Oct 7, 2022
Merged

[cmake] Fixes to build on Silicon #1241

merged 9 commits into from
Oct 7, 2022

Conversation

simogasp
Copy link
Member

@simogasp simogasp commented Sep 27, 2022

Here are some fixes to build on Apple Silicon. They are mostly version bumps of the dependencies that have added support for the Silicon platform.

  • bump geogram v1.7.6 -> v1.7.9
  • bump png v1.6.35 -> 1.6.37
  • some cmake adjustments to be platform independent

There are also some other bumps that I did not push yet:

  • Tbb 2019_U6 -> v2021.3.0. It normally works, even for CCTag using the oneAPI branch (TBB:2021 oneAPI compatibility CCTag#178). But I don't know if there were other reasons for keeping the previous version
  • for suitesparse no bump needed but for some reason I had to remove CC=${CMAKE_C_COMPILER} CXX=${CMAKE_CXX_COMPILER} from its build command line

@simogasp simogasp added this to the 2.5.0 milestone Sep 27, 2022
CMakeLists.txt Outdated
@@ -416,11 +424,11 @@ ExternalProject_Add(${PNG_TARGET}
BINARY_DIR ${BUILD_DIR}/png_build
INSTALL_DIR ${CMAKE_INSTALL_PREFIX}
# CONFIGURE_COMMAND <SOURCE_DIR>/configure --prefix=<INSTALL_DIR>
CONFIGURE_COMMAND ${CMAKE_COMMAND} ${CMAKE_CORE_BUILD_FLAGS} ${ZLIB_CMAKE_FLAGS} -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR> <SOURCE_DIR>
CONFIGURE_COMMAND ${CMAKE_COMMAND} -DPNG_ARM_NEON=off ${CMAKE_CORE_BUILD_FLAGS} ${ZLIB_CMAKE_FLAGS} -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR> <SOURCE_DIR>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be set to PNG_ARM_NEON=on. The build without the flag fails because it tries to enable png NEON detection which is not portable and relies on user-supplied code which we don't have.

Copy link
Member Author

@simogasp simogasp Sep 27, 2022

Choose a reason for hiding this comment

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

And setting it to on when building on UNIX does not give problems?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, I missed that this is common code with x86.

Copy link
Member Author

@simogasp simogasp Sep 27, 2022

Choose a reason for hiding this comment

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

so maybe have a variable set like

if(${CMAKE_SYSTEM_PROCESSOR} MATCHES "arm") 
    set(AV_PNG_ARM_NEON on)
else()
    set(AV_PNG_ARM_NEON off)
endif()

so we can put -DPNG_ARM_NEON=${AV_PNG_ARM_NEON}
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would make sense. I didn't suggest it because there're issues with CMAKE_SYSTEM_PROCESSOR - https://gitlab.kitware.com/cmake/cmake/-/issues/22881. Thinking about this more I think we should be safe with your approach because if the detection fails then we'll just use set(AV_PNG_ARM_NEON off).

@p12tic
Copy link
Contributor

p12tic commented Sep 27, 2022

LGTM, I pretty much had to implement the same fixes in my own builds scripts that don't use ExternalProject_ADD.

@fabiencastan fabiencastan merged commit 4680b27 into develop Oct 7, 2022
@fabiencastan fabiencastan deleted the dev/osxfixes branch October 7, 2022 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants