Skip to content

[libpng] Fix cross-compilation on macOS#15002

Merged
BillyONeal merged 1 commit intomicrosoft:masterfrom
orudge:fix-libpng-arm64-macos
Dec 11, 2020
Merged

[libpng] Fix cross-compilation on macOS#15002
BillyONeal merged 1 commit intomicrosoft:masterfrom
orudge:fix-libpng-arm64-macos

Conversation

@orudge
Copy link
Copy Markdown
Contributor

@orudge orudge commented Dec 8, 2020

libpng does not build correctly for arm64 on macOS, as the CMakeLists.txt supplied with libpng checks CMAKE_SYSTEM_PROCESSOR. vcpkg currently uses an x86_64 version of cmake, and cmake always sets CMAKE_SYSTEM_PROCESSOR to x86_64 in this situation, when run under Rosetta. Updated versions of cmake (see #15001) do report arm64 correctly, but in this case if we're cross-compiling we are not wanting to know the system processor, but the target architecture.

Ideally a fix may be submitted upstream, but for the time being I propose this patch, which should only be applied for macOS triplets, and will detect the target architecture and will then compile libpng with appropriate hardware-specific optimisations.

@JackBoosY JackBoosY self-assigned this Dec 9, 2020
@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label Dec 9, 2020
@JackBoosY
Copy link
Copy Markdown
Contributor

I think we need @strega-nil to review this PR. Because she is more familiar with MacOS.

@JackBoosY JackBoosY requested a review from strega-nil December 9, 2020 07:12
Comment on lines +33 to +35
if(VCPKG_CMAKE_SYSTEM_NAME STREQUAL Darwin)
set(MACOS_EXTRA_PATCH macos-arch-fix.patch)
endif()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be an optional patch, you can just do use it regardless.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would that not affect building on other platforms? The changes I made would only check an OSX-specific variable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, I see what you're saying.

Copy link
Copy Markdown
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

Noting that, unfortunately, this means that this doesn't support universal binaries. We need to figure out how we want to solve that problem.

@orudge
Copy link
Copy Markdown
Contributor Author

orudge commented Dec 9, 2020

Noting that, unfortunately, this means that this doesn't support universal binaries. We need to figure out how we want to solve that problem.

Yes, creating a universal triplet was my first thought, but I kept facing issues with libraries that compiled different files for different architectures. For my app, I basically need to perform two builds then lipo them together.

@JackBoosY
Copy link
Copy Markdown
Contributor

@strega-nil Anything else?

@orudge orudge force-pushed the fix-libpng-arm64-macos branch from 7c7939e to 6d5eb68 Compare December 10, 2020 11:19
@orudge orudge requested a review from JackBoosY December 10, 2020 11:22
@strega-nil
Copy link
Copy Markdown
Contributor

@JackBoosY I've approved it.

@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Dec 11, 2020
@BillyONeal BillyONeal merged commit 234227c into microsoft:master Dec 11, 2020
@BillyONeal
Copy link
Copy Markdown
Member

Thanks!

@orudge orudge deleted the fix-libpng-arm64-macos branch December 11, 2020 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-bug The issue is with a library, which is something the port should already support info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants