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

Fix demo compile issues due to CMake issues finding threading library #1399

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

Conversation

voutilad
Copy link
Contributor

@voutilad voutilad commented Jan 3, 2021

The demos weren't using the CMake FindThreads module to properly identify the threading library for use, which caused both CMake warnings like:

CMake Warning (dev) at CMakeLists.txt:19 (add_executable):                            
  Policy CMP0028 is not set: Double colon in target name means ALIAS or               
  IMPORTED target.  Run "cmake --help-policy CMP0028" for policy details.             
  Use the cmake_policy command to set the policy and suppress this warning.           
                                           
  Target "server" links to target "Threads::Threads" but the target was not           
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or         
  an ALIAS target is missing?                                                         
This warning is for project developers.  Use -Wno-dev to suppress it.

And resulted in broken builds where (at least on pthread-based platforms), bogus library args were being passed to the C compiler (-lThreads::Threads):

FAILED: server                                                                        
: && /usr/bin/cc   -rdynamic CMakeFiles/server.dir/server.c.o  -o server  /usr/local/lib/libnng.a -lThreads::Threads -lrt -lpthread -lnsl -latomic && :                      
/usr/bin/ld: cannot find -lThreads::Threads

This updates each demo to include the FindThreads to resolve both the warning and the broken builds.

Note: While in here, I also bumped the CMake version required to match the nng library itself.

@voutilad
Copy link
Contributor Author

voutilad commented Jan 3, 2021

Tested manually on Windows 10 and, assuming the nng library is available for CMake to find, the addition of include(FindThreads) doesn't cause an issue.

@gdamore
Copy link
Contributor

gdamore commented Jan 3, 2021

This doesn't feel right. I would have expected that the required threads library would have been pulled in via the nng library. What version of cmake were you using?

@gdamore
Copy link
Contributor

gdamore commented Jan 3, 2021

Maybe we need to have the include for FindThreads in the code that is imported. Folks using this library shouldn't have to worry about any of this.

@gdamore
Copy link
Contributor

gdamore commented Jan 3, 2021

I think we should probably just add FindThreads to the nng-config.cmake.in file.

@gdamore gdamore self-requested a review January 3, 2021 23:22
demo/async/CMakeLists.txt Outdated Show resolved Hide resolved
demo/async/CMakeLists.txt Outdated Show resolved Hide resolved
demo/async/CMakeLists.txt Outdated Show resolved Hide resolved
@voutilad
Copy link
Contributor Author

voutilad commented Jan 3, 2021

Should the demos be building when someone builds the library? Or are you saying the demos should be able to build relying on the local library source?

@gdamore
Copy link
Contributor

gdamore commented Jan 3, 2021

Demos should be able to be built as if they were out of tree -- they represent an example for consumers.

I should probably work up a CI/CD recipe to ensure that demos are buildable at least...

@gdamore
Copy link
Contributor

gdamore commented Jan 4, 2021

I can probably fix this properly if you prefer -- I've already looked at what is needed. But if you want to work the issue I'm happy to leave it in your hands as well.

@voutilad
Copy link
Contributor Author

voutilad commented Jan 4, 2021

I’ll take a stab at it. Need to brush up on CMake.

The generated nng-config.cmake file wasn't including
dependencies (like Threads) causing the demos to not build properly on
systems relying on pthreads. Version info was also not being
populated, but it seems that's been the case for at least the last release.
set(NNG_VERSION_MAJOR "@NNG_VERSION_MAJOR@")
set(NNG_VERSION_MINOR "@NNG_VERSION_MINOR@")
set(NNG_VERSION_PATCH "@NNG_VERSION_PATCH@")
set(NNG_VERSION_MAJOR "@NNG_MAJOR_VERSION@")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These haven't been populating properly for quite some time. I'm not sure if it's useful having these properties or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOl. :-)

@voutilad
Copy link
Contributor Author

voutilad commented Jan 4, 2021

@gdamore this approach seems to be working for me in my limited testing, at least making it so the demos build without modification. Question though: what optional external dependencies should be potentially included in the NNG_PKGS list? Threads is obvious and I've added zerotiercore, but should mbedTLS be in there as well if that's enabled?

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