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

add CMake config file that follows CMake conventions #545

Closed
wants to merge 1 commit into from

Conversation

bebuch
Copy link
Contributor

@bebuch bebuch commented Feb 15, 2024

New functionality:

  • add a CMake config file to the install, so an installed libpng can be found via find_package(libpng)
  • mark API header includes as SYSTEM headers in install, so warnings in these headers can be disabled

Changed behavior:

  • export the target libpng as libpng::libpng which follows the CMake convention for exported targets (previous it was just libpng)
  • install CMake files to lib/cmake/libpng instead of lib/libpng to follow the CMake convention

Copy link
Member

@ctruta ctruta left a comment

Choose a reason for hiding this comment

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

The code looks good overall -- but see my comment about the libpng vs. PNG namespace.

I would also like to have one commit per one task, submitted on top of the tree, as a general rule. We are sticking to a linear history, going forward, and we reserve the branches for the time when we fork new libpng versions. Could you please squash all commits, then rebase?

Last, if you could please update scripts/cmake/AUTHORS.md 🙂

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@bebuch bebuch force-pushed the libpng16 branch 2 times, most recently from 4fd8310 to 908dcf9 Compare March 1, 2024 12:37
@bebuch
Copy link
Contributor Author

bebuch commented Mar 1, 2024

I have now changed the PR so that it creates the config files I wanted and does not touch anything else. Ready to merge from my side! 😸

@bebuch
Copy link
Contributor Author

bebuch commented Jul 22, 2024

@ctruta If you find some time to work on libpng, it would be nice if you could merge this. That way we can use 1.6.44 without a patch once that is tagged. 🙂

@bebuch
Copy link
Contributor Author

bebuch commented Sep 7, 2024

@ctruta Since I just saw that you are active again, it would be nice if you could merge this ;-)

@ctruta
Copy link
Member

ctruta commented Sep 8, 2024

@ctruta Since I just saw that you are active again, it would be nice if you could merge this ;-)

My apologies for taking so long, and my appreciations for your patience. This PR is next in line.

@ctruta
Copy link
Member

ctruta commented Sep 8, 2024

@bebuch Integrated in master, and many thanks. This is super-useful.

Please don't mind me for modifying your commit directly, instead of asking you to do it in a review. After so much wait, now a lot of people want me to push a new release, sooner instead of later. I hope that you accept my modification -- BUT if you do have objections, feel free to submit another PR.

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.

3 participants