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 libpng:: namespace #381

Open
wants to merge 1 commit into
base: libpng16
Choose a base branch
from
Open

Conversation

h3ndrk
Copy link

@h3ndrk h3ndrk commented May 25, 2021

This PR adds a libpng:: namespace to CMakeLists.txt. I'm pasting/quoting the answer of https://stackoverflow.com/a/48526017:

The cmake-developer documentation gives the following advice on namespaces:

When providing imported targets, these should be namespaced (hence the Foo:: prefix); CMake will recognize that values passed to target_link_libraries() that contain :: in their name are supposed to be imported targets (rather than just library names), and will produce appropriate diagnostic messages if that target does not exist (see policy CMP0028).

And the CMP0028 policy documentation says on the "common pattern" in the use of namespaces:

The use of double-colons is a common pattern used to namespace IMPORTED targets and ALIAS targets. When computing the link dependencies of a target, the name of each dependency could either be a target, or a file on disk. Previously, if a target was not found with a matching name, the name was considered to refer to a file on disk. This can lead to confusing error messages if there is a typo in what should be a target name.

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.

Thank you, Hendrik, for your contribution.

Could you please add your full name at the top of the CMakeLists.txt file, near the names of the other contributors?

Could you please also amend your commit so that your full name appears in the Author and Committer field?

As a general rule, I would like all future libpng contributors to include their full names, for copyright purposes.

@h3ndrk
Copy link
Author

h3ndrk commented Sep 13, 2022

Regarding your new general rule, I'm not very happy that I am forced to expose my full name in public. Also I don't see any reason adding names to files because that's the whole reason why we have git. Nevertheless, since I would like to finally get this merged, I implemented your requested changes. In addition, I rebased to the latest master. I hope everything is covered from my side.

@theta682
Copy link
Contributor

@ctruta probably we can improve CMake packaging see #475 too. At the moment the standard FindPNG Module can't properly find headers because it expects them in include/libpng instead of include/libpng16

@ctruta
Copy link
Member

ctruta commented Sep 8, 2024

@h3ndrk @theta682 I'm very sorry for not fully understanding the compatibility implications of your PR. I wish I knew more about CMake than I actually do.

I specifically could not understand what will happen if I changed the namespace, from the global one, to libpng::. Now that we are preparing for a new version line, post-1.6.x, I guess that the PNG:: namespace could be added. (Please note: the namespace is PNG:: not libpng::.

@bebuch FYI ^

@h3ndrk
Copy link
Author

h3ndrk commented Nov 20, 2024

@ctruta How shall we proceed? Should I rename the prefix to ::PNG and rebase to the latest master or something else? Or did you already implement it in your 1.6 release preparations? Please give more concrete instructions, thanks! 😊

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