feat: Adds conan support via a conan 2.0 recipe #1066

MikeRavenelle · 2024-01-19T23:27:05Z

Added a conan 2.0 recipe
Modified the CMakeLists.txt to check for presence of conan_toolchain.cmake and include libs accordingly(for building with or without conan)
Added .md file for conan usage

I went ahead and tested the conan package on macOS and linux with example programs(using conan). I also tested the existing build procedure on macOS to make sure I did not break anything. I did no testing on Windows as I do not currently have a Windows development machine ready for use.

I would personally like to see more testing done on other users machines before a package is submitted to the conan center.

A review of my CMakeLists.txt changes also is not be a bad idea since I will admit I am not a CMake expert.

How does versioning work? I used 0.1 as a placeholder until I got feedback.

Thanks!

Code change checklist

I have ensured that all methods and functions are fully documented using doxygen style comments.
My code follows the coding style guide.
I tested that my change works before raising the PR.
I have ensured that I did not break any existing API calls.
I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

netlify · 2024-01-19T23:27:11Z

✅ Deploy Preview for dpp-dev ready!

Name	Link
🔨 Latest commit	`2f02ae7`
🔍 Latest deploy log	https://app.netlify.com/sites/dpp-dev/deploys/6606bd31f3fcb60008b18950
😎 Deploy Preview	https://deploy-preview-1066--dpp-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

CLAassistant · 2024-01-19T23:27:12Z

All committers have signed the CLA.

…into conan_support

… CMakeLists.txt, root CMakeLists now detects conan and uses its cmake if detected

…CE_SUPPORT by default

…ry-conan folder

Jaskowicz1 · 2024-01-25T11:30:46Z

Can we automate the versioning to match the ${DPP_VERSION} cmake var? This will help automate the process. If not, put it to our latest release (10.0.29) :)

Jaskowicz1 · 2024-01-25T13:00:31Z

I've also added the "Help Wanted" label on this PR for anyone who wants to test the conan recipe (more specially on Windows).

Like you said, we need people to test the conan recipe so asking for people to join in and test is ideal.

docpages/install/install-conan.md

…penSSL and ZLIB since they are not needed with conan

MikeRavenelle · 2024-01-27T00:47:45Z

Currently some issues getting it to build on Windows. I am not a windows developer so any help is appreciated.

…folder

braindigitalis · 2024-02-03T05:22:39Z

-1073741819 is error C0000005 which is like a segmentation fault.
It's likely something you have installed is not set up correctly or your system has a hardware issue.

braindigitalis · 2024-02-19T10:31:40Z

any movement on this?

Jaskowicz1 · 2024-03-28T16:57:53Z

Renamed PR to correctly align with our conventions.

Jaskowicz1 · 2024-03-28T17:02:22Z

I'm going to run some tests on this PR for windows, just to make sure it all works.

Jaskowicz1 · 2024-03-28T20:59:17Z

library-conan/conanfile.py

+        deps.generate()
+        tc = CMakeToolchain(self)
+        tc.cache_variables["CONAN_EXPORTED"] = True
+        tc.cache_variables["BUILD_VOICE_SUPPORT"] = True


Add

tc.cache_variables["DPP_BUILD_TEST"] = False tc.cache_variables["BUILD_SHARED_LIBS"] = False

to this and windows support will work! (DPP_BUILD_TEST is unrelated, it's just to save space).

Jaskowicz1 · 2024-03-29T00:11:52Z

Even with the suggested changes, it appears Conan just can't find D++ (despite all the files being there and cmake loading all the variables).

I end up getting:

-- Selecting Windows SDK version 10.0.22000.0 to target Windows 10.0.22631.
-- Conan: Target declared 'dpp::dpp'
CMake Error at build/cmakedeps_macros.cmake:67 (message):
  Library 'dpp' not found in package.  If 'dpp' is a system library, declare
  it with 'cpp_info.system_libs' property
Call Stack (most recent call first):
  build/dpp-Target-release.cmake:23 (conan_package_library_targets)
  build/dppTargets.cmake:24 (include)
  build/dpp-config.cmake:16 (include)
  CMakeLists.txt:10 (find_package)

As far as I understand, it's a Conan bug? I could be super wrong but I really don't understand.

memsharded

I'd suggest using the test_package functionality that can test automatically the package when created with conan create .

memsharded · 2024-03-29T21:29:32Z

library-conan/conanfile.py

+    options = {"shared": [True, False], "fPIC": [True, False]}
+    default_options = {"shared": False, "fPIC": True}
+
+    def build_requirements(self):


Suggested change

def build_requirements(self):

def requirements(self):

those are regular requires, recommended to go on the requirements() method

memsharded · 2024-03-29T21:30:49Z

library-conan/conanfile.py

+        cmake.install()
+
+    def package_info(self):
+        self.cpp_info.libs = ["dpp"]


As commented in conan-io/conan#15985 (comment) this might need more info, as the folders generated in cmake.install() are not matching the defaults.

memsharded · 2024-03-29T21:31:38Z

library-conan/conanfile.py

+    default_options = {"shared": False, "fPIC": True}
+
+    def build_requirements(self):
+        self.requires("nlohmann_json/3.11.2", headers=True, libs=True)


headers=True, lib=True is the default, it is recommended to drop them and use self.requires()

memsharded · 2024-03-29T21:32:20Z

library-conan/conanfile.py

+        tc.cache_variables["CONAN_EXPORTED"] = True
+        tc.cache_variables["BUILD_VOICE_SUPPORT"] = True
+        tc.cache_variables["DPP_BUILD_TEST"] = False
+        tc.cache_variables["BUILD_SHARED_LIBS"] = False


If it is always going to be a static library, then better drop the shared option and declare package_type = "static-library"

Yeah this one is an odd one, we normally don't allow static but for some reason, trying to build shared results in ZLib linking twice.

I do believe we would like to build as a shared library (as this is what we do normally) but, as seen, are kinda forced into static due to ZLib shenanigans.

It might be in Conan 1.X, that it was overlinking (propagating too much linkage requirements). In theory, Conan 2 with the right traits/package_type, Conan should avoid this overlinking, and allow shared builds to encapsulate zlib static libs as implementation detail if desired. Feel free to submit new tickets to Conan repo for further details or discussion.

It might be in Conan 1.X, that it was overlinking (propagating too much linkage requirements). In theory, Conan 2 with the right traits/package_type, Conan should avoid this overlinking, and allow shared builds to encapsulate zlib static libs as implementation detail if desired. Feel free to submit new tickets to Conan repo for further details or discussion.

Sorry for the delayed reply, the zlib issue has been a thing as long as Mike has been trying to make this PR (I mentioned it in the issue that I posted on the conan page too!). When I then tested this PR a few days ago, it was still persisting on Conan 2.2.2.

I think it would be great to isolate this issue, create a ticket for it in the Conan repos (hopefully with something that we can reproduce on our end) and investigate this properly.

memsharded · 2024-03-29T21:34:34Z

library-conan/CMakeLists.txt

+message("-- INFO: Conan detected... finding packages")
+find_package(OpenSSL REQUIRED COMPONENTS SSL Crypto)
+find_package(ZLIB REQUIRED)
+find_package(libsodium REQUIRED)
+find_package(Opus)
+find_package(Threads REQUIRED)
+find_package(Git QUIET)
+


I understand that using Conan might need some find_package() that maybe are not used when not using Conan because some other strategy like FetchContent is done, but why the big CMakeLists.txt and all the code below? In theory Conan can integrate quite cleanly, without requiring modifying CMakeLists code.

We wanted the CMakeLists.txt to be separate (see library-vcpkg), this is just something we do with VCPKG so we wanted the same format to follow.

It should also be noted, our main CMakeLists.txt doesn't do everything hence why we have library/CMakeLists.txt, library-vcpkg/CMakeLists.txt, and now library-conan/CMakeLists.txt.

What is a bit unexpected to me is that such a file wouldn't be way shorter, at least for Conan. Besides some potential minor differences in the dependencies (like the above commented possible find_package() vs FetchContent). So it shouldn't be necessary much different logic to build the project. There are full blocks of CMake code in library-conan/CMakeLists.txt that are completely unrelated to Conan, and also seem completely unrelated to dependencies too, basic definition of the library like globbing files, handling compiler flags, etc. So I would expect that this would be common CMakeLists.txt, and the specializations for vcpkg or Conan would be way more minimal.

Am I missing something here?

MikeRavenelle added 6 commits January 19, 2024 10:49

First test of SCM conan 2.0

e838136

Fixed requires

7277b21

Added package info for libs

60d31d4

renamed libdpp to dpp

5947864

Made CmakeLists work for both conan and non conan users

25943bb

Added documentation for using conan

f7ee95b

MikeRavenelle added 7 commits January 19, 2024 17:59

Merge branch 'dev' into conan_support

c2c2854

Changed how conan detected and changed how opus is checked

3abef91

Merge branch 'conan_support' of https://github.com/MikeRavenelle/DPP …

7e3a98b

…into conan_support

Added DPP_NO_CONAN to pipeline, moved conan cmake out of main library…

857c4fc

… CMakeLists.txt, root CMakeLists now detects conan and uses its cmake if detected

Reset ci.yml since github wont let me commit

0c5cbf0

Fixed 1 error and tabbing

84792c8

Added missing endif()

d5dd3f6

MikeRavenelle force-pushed the conan_support branch 3 times, most recently from a5aa313 to 0b193a1 Compare January 21, 2024 15:29

Added parent folder for commit instead of recipe folder and BUILD_VOI…

ffa9cd5

…CE_SUPPORT by default

MikeRavenelle force-pushed the conan_support branch from 0b193a1 to ffa9cd5 Compare January 21, 2024 15:30

MikeRavenelle added 2 commits January 21, 2024 09:36

Fixed CMake variables for opus and libsodium

f4ecfc3

Removed load from conan.tools.files and moved conan recipe into libra…

68560d6

…ry-conan folder

Jaskowicz1 added build Issue or Pull Request related to the build process packages Issue or Pull Request related to packaging help wanted Extra attention is needed labels Jan 25, 2024

Updated the version number

f37ce14

braindigitalis requested changes Jan 25, 2024

View reviewed changes

docpages/install/install-conan.md Outdated Show resolved Hide resolved

Remove docpage from branch so we can control docpage separately.

ea84883

Rearranged find_packages to top and removed include directories for O…

3b50591

…penSSL and ZLIB since they are not needed with conan

Removed possibly uneeded windows flags and added exclusion for win32 …

de0c141

…folder

braindigitalis approved these changes Feb 3, 2024

View reviewed changes

Jaskowicz1 changed the title ~~Adds conan support via a conan 2.0 recipe~~ feat: Adds conan support via a conan 2.0 recipe Mar 28, 2024

Jaskowicz1 requested changes Mar 28, 2024

View reviewed changes

Added flags for tests and BUILD_SHARED_LIBS

2f02ae7

memsharded reviewed Mar 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds conan support via a conan 2.0 recipe #1066

feat: Adds conan support via a conan 2.0 recipe #1066

MikeRavenelle commented Jan 19, 2024

netlify bot commented Jan 19, 2024 •

edited

CLAassistant commented Jan 19, 2024 •

edited

Jaskowicz1 commented Jan 25, 2024

Jaskowicz1 commented Jan 25, 2024

MikeRavenelle commented Jan 27, 2024

braindigitalis commented Feb 3, 2024

braindigitalis commented Feb 19, 2024

Jaskowicz1 commented Mar 28, 2024

Jaskowicz1 commented Mar 28, 2024

Jaskowicz1 Mar 28, 2024 •

edited

Jaskowicz1 commented Mar 29, 2024

memsharded left a comment

memsharded Mar 29, 2024

memsharded Mar 29, 2024

memsharded Mar 29, 2024

memsharded Mar 29, 2024

Jaskowicz1 Mar 29, 2024 •

edited

memsharded Mar 31, 2024

Jaskowicz1 Apr 3, 2024

memsharded Apr 3, 2024

memsharded Mar 29, 2024

Jaskowicz1 Apr 3, 2024

Jaskowicz1 Apr 3, 2024 •

edited

memsharded Apr 3, 2024

feat: Adds conan support via a conan 2.0 recipe #1066

Are you sure you want to change the base?

feat: Adds conan support via a conan 2.0 recipe #1066

Conversation

MikeRavenelle commented Jan 19, 2024

Code change checklist

netlify bot commented Jan 19, 2024 • edited

✅ Deploy Preview for dpp-dev ready!

CLAassistant commented Jan 19, 2024 • edited

Jaskowicz1 commented Jan 25, 2024

Jaskowicz1 commented Jan 25, 2024

MikeRavenelle commented Jan 27, 2024

braindigitalis commented Feb 3, 2024

braindigitalis commented Feb 19, 2024

Jaskowicz1 commented Mar 28, 2024

Jaskowicz1 commented Mar 28, 2024

Jaskowicz1 Mar 28, 2024 • edited

Choose a reason for hiding this comment

Jaskowicz1 commented Mar 29, 2024

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jaskowicz1 Mar 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jaskowicz1 Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jan 19, 2024 •

edited

CLAassistant commented Jan 19, 2024 •

edited

Jaskowicz1 Mar 28, 2024 •

edited

Jaskowicz1 Mar 29, 2024 •

edited

Jaskowicz1 Apr 3, 2024 •

edited