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

Patch to allow building libpng on Macs with modern build system #1946

Closed

Conversation

slingthor
Copy link
Contributor

Description of Change(s)

Patches the building of libpng to work around an error when building with the modern build system.
Prevents the command generating pnglibconf.c from being attached to the following build targets:
genvers
gensym
genfiles

Since none of these targets are are common dependency of each other, this causes an error and is not allowed by the Xcode modern build system.

Fixes Issue(s)

-Unable to build libpng on MacOS with modern build system

  • I have submitted a signed Contributor License Agreement

@meshula
Copy link
Member

meshula commented Jul 11, 2022

Please note that there is a fix here - pnggroup/libpng#403 - although it seems unlikely to land in a timely manner. Sadly the author of libpng passed away, and there aren't currently active maintainers (brief discussion here: pnggroup/libpng#410)

@slingthor
Copy link
Contributor Author

Please note that there is a fix here - glennrp/libpng#403 - although it seems unlikely to land in a timely manner. Sadly the author of libpng passed away, and there aren't currently active maintainers (brief discussion here: glennrp/libpng#410)

I agree the best way to work around it would be at the libpng level. Yea had heard about the author passing away, that's really sad.
Hopefully this patch can help until then. Also, is there a way to retrigger the test runner? I amended my commit and believe it may have confused it.

("add_custom_target(genfiles DEPENDS",
"add_custom_target(genfiles DEPENDS gensym symbol-check")])

RunCMake(context, force, buildArgs)
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the "+ macArgs" that is needed to pass the Neon optimization args for Arm builds down to RunCMake().

@slingthor slingthor force-pushed the libpng-building-on-mac branch 2 times, most recently from 9033722 to 93a5dd3 Compare July 12, 2022 21:26
@slingthor
Copy link
Contributor Author

Thanks for the valuable feedback so far guys! I've updated the PR to reflect the comments made!

@slingthor
Copy link
Contributor Author

We've come to notice, that perhaps this is only happening for those who generate the project with --generator Xcode.
We can confirm, that if we use Jinja this does not occur.
The issue might be a full on blocker for generating with Xcode

@sunyab
Copy link
Contributor

sunyab commented Jul 18, 2022

Filed as internal issue #USD-7489

starseeker added a commit to BRL-CAD/brlcad that referenced this pull request Aug 19, 2022
It looks as if there's an issue with the CMake code for PNG with modern
XCode:
PixarAnimationStudios/OpenUSD#1946
pnggroup/libpng#403

This commit makes a stab at adapting the version from
libpng/libpng#2

onto our (somewhat simplified) png build.

It looks as if libpng doesn't currently have much in the way of an
active upstream (no new releases in a number of years), so will need to
keep an eye out to see what the eventual solution is.
@meshula
Copy link
Member

meshula commented Sep 10, 2022

@slingthor @davidgyu pnggroup/libpng#403 has landed!

@creijon
Copy link
Contributor

creijon commented Nov 10, 2022

@slingthor @davidgyu glennrp/libpng#403 has landed!

With the libPNG upgrade to v1.6.38 in #1959 this patch isn't required.

@slingthor slingthor closed this Nov 14, 2022
@slingthor
Copy link
Contributor Author

Closed as no longer needed

pixar-oss pushed a commit that referenced this pull request Apr 3, 2023
This version includes improved support for Xcode in addition
to other maintenance fixes.

Contribution: Jon Creighton

Fixes #1946

(Internal change: 2268616)
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.

5 participants