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

Allow and demonstrate NNG usage without elevated privilege #1902

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dksmiffs
Copy link

@gdamore, this PR breaks two CONTRIBUTING rules, but for these reasons:

  1. I couldn't squash to one commit, because the second commit needs the SHA1 of the first.
  2. If you accept this change, a follow-up change will be required to the new demo's CMakeLists.txt, to refer to your repo instead of my fork.

I hope you understand this awkwardness was necessary because the change I'm suggesting isn't in your repo yet, and yet my demo's CMakeLists.txt requires SHA1 access to the repo itself with the single line suggested change to the top-level CMakeLists.txt.

I promise I will respond to review feedback, and not leave this baby on your doorstep.

Doesn't fix nanomsg#1582 in general, rather suggests one way that demo builds
could be more self contained. This commit also encourages a CMake approach
for downstream clients of NNG that is gaining widespread acceptance,
evidence given in the README.
@gdamore
Copy link
Contributor

gdamore commented Nov 3, 2024

So lets fix this to refer to the main NNG repo instead. Let me see if I can take this work and adjust it to work that way.

@dksmiffs
Copy link
Author

dksmiffs commented Nov 5, 2024

@gdamore, after more thought, I don't like the fact that I've overloaded the meaning of nng::nng. The nng::nng that I introduced in the top level CMakeLists.txt file is a CMake library alias. That's not the same thing as the value of the NNG_LIBRARY variable you have set to nng::nng in cmake/nng-config.cmake.in. Maybe those overloaded names wouldn't conflict, but I don't have enough experience with CMake config packages to say for sure.

I would feel much more confident if we renamed the library alias I introduced to something unique like nng::nnglib. If you'd be okay with this tweak, I'd be happy to make the change in this pull request.

Instead of rushing to a solution, I will seek out guidance on CMake Discourse first, and come back here with an updated, more informed suggestion.

@gdamore
Copy link
Contributor

gdamore commented Nov 5, 2024

@dksmiffs I'm far from a CMake expert myself. In fact, I've got plans to move away from using cmake as the "primary" build system (moving towards meson and maybe then generating the CMake from meson inputs.)

I'll take a look at your concerns ... I want to think on it before I make a ruling. :-)

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.

2 participants