RSDK-7861 Conan dependencies #283

lia-viam · 2024-08-19T21:14:09Z

Dependency management with conan--provides a recipe for packaging the SDK with conan and some examples/docs to support

Currently tested on mac as well as bullseye docker container, featuring configuration options listed in the options of the conanfile.py

TBD:

building in CI
test_package that assesses if static builds are working

…ure/conan-deps

acmorrow

This looks like a great start, various questions and suggestions.

CMakeLists.txt

conanfile.txt

src/viam/api/CMakeLists.txt

src/viam/api/config/invoke_buf_generate.cmake.in

src/viam/api/CMakeLists.txt

acmorrow · 2024-08-23T14:30:58Z

@lia-viam - Is this ready for another round of review?

lia-viam · 2024-09-09T14:45:30Z

@acmorrow still needs work! last commits were just what I managed to fit in before vacation

lia-viam · 2024-09-10T20:39:18Z

@acmorrow just re-requested a review, this is now in a state I'm more proud of. Before merging I would like to think about doing some of the following in CI

Using conan create . to make sure the package and its test package build successfully
Building a second version of the CMake example project in CI
Possibly for releases only? Testing conan create . with the full CI matrix

The possible pain point here is that we are already building from source, but then conan create . will build the world anew, but this is needed to test the example project which I think we want to be doing to prevent code rot like with the previous examples.

During development on this I kept wishing for RSDK-4735 so it might be nice to get that in soon, and would make it a bit less onerous to do the double build if that's the route we go.

We also could just make it so conan stuff is a separate CI job which only gets built on releases.

hexbabe · 2024-09-12T18:15:50Z

BUILDING.md

@@ -148,6 +148,121 @@ an installation tree under `build/install`.

 If the build did not succeed, please see the next section.

+## Building with `conan`


I didn't know what conan is. I think it would be nice to add a docs link closer to the beginning (maybe hyperlinked to its first mention) so users can familiarize themselves with the 3rd party pkg

(I initially read canon like James' docker thing haha)

hexbabe · 2024-09-12T18:20:25Z

BUILDING.md

+while using `conan` to get dependencies instead of your system package
+manager. Note that Option 1 implies a superset of Option 2.
+
+### Creating and consuming the SDK `conan` package.


This is option 1 right? Can we do Option 1: Creating and consuming the SDK conan package.

hexbabe · 2024-09-12T18:20:36Z

BUILDING.md

+or look at the [`test_package/conanfile.py`](test_package/conanfile.py)
+which is the test package recipe.
+
+### Using `conan` to manage the SDK dependencies


Same thing for Option 2:

hexbabe · 2024-09-12T18:22:15Z

BUILDING.md

+Note that this can be done with the same `CMakeLists.txt` from the
+[example project](src/viam/examples/project/cmake/CMakeLists.txt): it is
+agnostic of the use of `conan` to package the SDK as opposed to the SDK
+having been built and installed manually.


Is this due to the flexibility of using find_package

Pretty much yeah! In the snippet above when you do cmake --preset=conan-release that's where conan makes it so that find_package will know about the packages it has installed, but all of that takes place on the command line rather than intrusively in the CMakeLists.txt itself

lia-viam · 2024-09-12T18:46:09Z

@hexbabe thanks for the doc comments, I've pushed some changes!

acmorrow

This looks really good. All small things / questions and a few small suggestions. My only real concern is re the hardcoded version numbers.

CMakeLists.txt

src/viam/api/CMakeLists.txt

CMakeLists.txt

acmorrow · 2024-09-18T20:39:44Z

conanfile.py

+
+class ViamCppSdkRecipe(ConanFile):
+    name = "viam-cpp-sdk"
+    version = "0.0.10"


There will need to be something in the release process to advance this number, or it will surely drift. Can we derive it somehow?

Yeah this was bugging me as well and I stumbled on this in the old 1.x conan docs:
https://docs.conan.io/en/1.4/howtos/capture_version.html
Going to ask online if this is still considered acceptable

If there isn't a good way, we can always doc it in the release process.

Update: turns out set_version does what I needed

conanfile.py

acmorrow · 2024-09-18T20:42:40Z

src/viam/examples/project/cmake/conanfile.txt

@@ -0,0 +1,6 @@
+[requires]
+viam-cpp-sdk/0.0.10


Ditto re version

Ok so this part I think we might not be able to derive the version programatically. In keeping with the spirit that stuff in the examples/project directory should be usable as starter code, a conanfile.txt is the easiest way to get started with a conan project that depends on the SDK.

A conanfile.py could in theory get the version like the root conanfile.py as sketched above, but then this would be grabbing a version from ../../../../CMakeLists.txt which would probably merit an all-caps warning comment to delete that in your own project and write in an explicit version.

We could specify a version range like viam-cpp-sdk/[>=0.0.12] (or whichever release this lands in) but I don't love that.

I think the last option is that if the SDK release instructions say "manually bump the version in the CML.txt and also the example project conanfile.txt" which might not be so terrible

Yeah, I hadn't really caught that this was in the example. I just saw a hardcoded version and the usual reflexes kicked in. I don't know: I sort of find the viam-cpp-sdk/[>=0.0.12] sort of intuitively appealing? Can you elaborate on why you don't love it?

I think it might be fine actually, looking at https://docs.conan.io/2/tutorial/versioning/version_ranges.html it seems like it will prefer to use the newer one if available, just trying to imagine a situation where the user is accidentally getting an old version because they hadn't yet built a new one

switched to using a version range!

acmorrow · 2024-09-18T20:43:38Z

src/viam/sdk/CMakeLists.txt

@@ -237,6 +237,8 @@ target_link_libraries(viamsdk
  PRIVATE absl::strings
  PRIVATE viam_rust_utils
  PRIVATE Threads::Threads
+  PRIVATE $<$<PLATFORM_ID:Linux>:dl>


Not that it matters, but I'd bet a shiny nickel these are needed on, say, BSD. Is there a more general platform tag than Linux?

I wanted to ask you about that, it comes up in the conanfile too. It seems to be idiomatic to do the following:
https://github.com/conan-io/conan-center-index/blob/master/recipes/grpc/all/conanfile.py#L302
And I figured we may as well do the same even if we don't claim in the README to support BSD. But yes I can do a Linux || BSD clause here and in the conanfile

In the sdk/CML.txt we do an if (APPLE) to link -framework Security so I may just spell this as an if (NOT APPLE)

Yeah, the reason for that -framework Security is interesting, btw: https://github.com/viamrobotics/viam-cpp-sdk/pull/146/files#r1334516668.

Essentially, it is viam_rust_utils that has a dependency on the Security framework on macOS. However, since we don't obtain viam_rust_utils via a find_package like thing, there is no way for it to express its transitive dependencies.

I'm not particularly happy about needing to do that, but I don't have a better answer.

Regarding your suggestion to make this just be "not apple", I think that makes sense. Maybe do "not apple and not windows?"

ahh that explains a lot, I think that's why I ended up needing to add librt and libdl too, I was getting linker errors from viam_rust_utils on the static build without them.

As for not windows, I don't think there is currently any mention of windows in our build files, are we hoping to be able to offer it down the road?

CMakeLists.txt

…file.txt

acmorrow

LGTM!

lia-viam added 7 commits July 23, 2024 11:47

update paths to cpp files

cdbd2e0

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk

764cf2f

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into feat…

29e5263

…ure/conan-deps

add conanfile

c17433f

add fallback FindProtobuf

feae24f

add wrapper to invoke buf generate with PATH variable set

f7fecd3

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into feat…

29b5fd9

…ure/conan-deps

lia-viam requested a review from acmorrow August 19, 2024 21:14

lia-viam requested a review from a team as a code owner August 19, 2024 21:14

lia-viam requested review from njooma and purplenicole730 and removed request for a team August 19, 2024 21:14

acmorrow reviewed Aug 20, 2024

View reviewed changes

lia-viam added 5 commits August 21, 2024 14:33

revert buf wrapper script

7e90f39

clarify find_package Protobuf

bd69a33

introduce conanfile.py

a552035

document conan support

84164eb

conditional tool_requires

2ff2b29

lia-viam added 6 commits September 10, 2024 11:54

flesh out conanfile

3edbe37

commit test_package

dd1d535

technically still on version 0.0.10

7616ea4

update docs and add example conanfile.txt

357309b

fix indent

f879c45

try to fix markdown relative link

c06e56b

lia-viam requested a review from acmorrow September 10, 2024 20:31

purplenicole730 removed their request for review September 11, 2024 20:00

require shared grpc

9d699c4

hexbabe reviewed Sep 12, 2024

View reviewed changes

clarify docs per code review

294fde7

lia-viam added 15 commits September 13, 2024 11:13

add commented out lines which break the build if uncommented

3cd71a2

fix shared and add transitive_headers

afbe493

fix dict key typo

dd3e844

add dl and rt for linux builds

8fdd40e

conditional pthread

6e4a5e8

less optimistic version

15fed98

fix version

30c9902

Allow selective building and installing of unit tests and examples

f5ec90a

disable tests and examples

76afc43

fix var name and simplify if stmt

6b5dd7d

always use_dynamic_protos

93a8c44

wip: make online generation work

f061993

remove online gen toggle

0da5216

fix online proto generation

146559e

update docs

149c50f

acmorrow requested changes Sep 18, 2024

View reviewed changes

lia-viam added 5 commits September 19, 2024 10:42

remove propagate_shared

ad2390a

get recipe version from CML.txt and use version range in sample conan…

affe08f

…file.txt

undo whitespace changes

7173e87

more explicit case handling for find grpc and proto

dd0058f

generalize system libs for non-apple

b11ee1e

lia-viam requested a review from acmorrow September 19, 2024 17:25

acmorrow approved these changes Sep 19, 2024

View reviewed changes

lia-viam changed the title ~~Draft: RSDK-7861 Conan dependencies~~ RSDK-7861 Conan dependencies Sep 19, 2024

update version number

81c9229

lia-viam merged commit fe83db6 into viamrobotics:main Sep 19, 2024
3 checks passed

lia-viam deleted the feature/conan-deps branch September 19, 2024 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-7861 Conan dependencies #283

RSDK-7861 Conan dependencies #283

lia-viam commented Aug 19, 2024 •

edited

Loading

acmorrow left a comment

acmorrow commented Aug 23, 2024

lia-viam commented Sep 9, 2024

lia-viam commented Sep 10, 2024 •

edited by jira bot

Loading

hexbabe Sep 12, 2024

hexbabe Sep 12, 2024

hexbabe Sep 12, 2024

hexbabe Sep 12, 2024

lia-viam Sep 12, 2024

lia-viam commented Sep 12, 2024

acmorrow left a comment

acmorrow Sep 18, 2024

lia-viam Sep 19, 2024

acmorrow Sep 19, 2024

lia-viam Sep 19, 2024

acmorrow Sep 18, 2024

lia-viam Sep 19, 2024

acmorrow Sep 19, 2024

lia-viam Sep 19, 2024

lia-viam Sep 19, 2024

acmorrow Sep 18, 2024

lia-viam Sep 19, 2024

lia-viam Sep 19, 2024

acmorrow Sep 19, 2024

lia-viam Sep 19, 2024

acmorrow left a comment

		@@ -148,6 +148,121 @@ an installation tree under `build/install`.

		If the build did not succeed, please see the next section.

		## Building with `conan`

		@@ -0,0 +1,6 @@
		[requires]
		viam-cpp-sdk/0.0.10

RSDK-7861 Conan dependencies #283

RSDK-7861 Conan dependencies #283

Conversation

lia-viam commented Aug 19, 2024 • edited Loading

acmorrow left a comment

Choose a reason for hiding this comment

acmorrow commented Aug 23, 2024

lia-viam commented Sep 9, 2024

lia-viam commented Sep 10, 2024 • edited by jira bot Loading

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

lia-viam commented Sep 12, 2024

acmorrow 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

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

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

acmorrow left a comment

Choose a reason for hiding this comment

lia-viam commented Aug 19, 2024 •

edited

Loading

lia-viam commented Sep 10, 2024 •

edited by jira bot

Loading