-
Notifications
You must be signed in to change notification settings - Fork 22
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
produce .deb with cpack #165
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I'd like to know more about the intended use case for debian packages for the C++ SDK is right now. Are we planning to offer them on our own apt repository? I'm somewhat doubtful that we are in a position to correctly manage versions, especially given that we have yet to produce any tagged releases of this project.
@@ -7,7 +7,6 @@ Name: @PROJECT_NAME@-libapi | |||
Description: @PROJECT_DESCRIPTION@ | |||
URL: @PROJECT_HOMEPAGE_URL@ | |||
Version: @PROJECT_VERSION@ | |||
Requires: gRPC++ >= @VIAMCPPSDK_GRPCXX_VERSION@ protobuf >= @VIAMCPPSDK_PROTOBUF_VERSION@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, these need to remain. Otherwise, projects using pkg-config
to consume the SDK will fail to link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just read your comment that said "remove Requires line -- it adds like a minute of CPU-bound processing inside the consumer's cmake config". I don't see how that could be true. These files aren't consumed by cmake, only by pkg-config
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example run with Requires
in pkg-config (last line has time):
# cmake .. -G Ninja
-- The CXX compiler identification is GNU 12.2.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found PkgConfig: /usr/bin/pkg-config (found version "1.8.1")
-- Checking for one of the modules 'viam-cpp-sdk-libviamsdk'
-- Checking for one of the modules 'viam-cpp-sdk-libviamapi'
-- Configuring done (31.1s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. But why are you using pkg-config
to obtain the SDK rather than using find_pacakge
, which will use the cmake infra that the SDK provides?
libdir=${prefix}/@CMAKE_INSTALL_LIBDIR@ | ||
includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@ | ||
includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@/viam/sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this was changed. The viam/api
tacked onto the include path for the libviamapi.pc.in
is peculiar to the viamapi
library, and is a side effect of some unfortunate naming issues in our api
repository. But the sdk
library doesn't have those problems, and the intended include root for the sdk
library is definitely the INSTALL_INCLUDEDIR
.
|
||
Name: @PROJECT_NAME@-libviamsdk | ||
Description: @PROJECT_DESCRIPTION@ | ||
URL: @PROJECT_HOMEPAGE_URL@ | ||
Version: @PROJECT_VERSION@ | ||
Requires: gRPC++ >= @VIAMCPPSDK_GRPCXX_VERSION@ protobuf >= @VIAMCPPSDK_PROTOBUF_VERSION@ @PROJECT_NAME@-libviamapi >= @PROJECT_VERSION@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- As above, I don't think these
Requires
can or should be removed. - I think this constraint on the
libviamapi
version should remain as well, but looking more closely it looks slightly wrong, in that it doesn't exactly match the name at src/viam/api/config/viam-cpp-sdk-libviamapi.pc.in. I think maybe that.pc.in
should haveName: @PROJECT_NAME@-lib**viam**api
instead.
src/viam/sdk/CMakeLists.txt
Outdated
set(CPACK_ARCHIVE_COMPONENT_INSTALL ON) | ||
set(CPACK_COMPONENTS_ALL viam-cpp-sdk_dev) | ||
set(CPACK_PACKAGE_CONTACT "[email protected]") | ||
set(CPACK_DEBIAN_PACKAGE_DEPENDS "libprotobuf-dev, libgrpc++-dev") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list looks pretty incomplete. We definitely have dependencies on other development libraries. Also, I'd be hoping to see a split between dependencies required for build, for runtime, and for development. Does cpack let us express those? I'd also be interested to know if cpack has any ability to auto-detect these package dependencies rather than requiring us to explicitly enumerate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the viam-cpp-sdk .deb is a development library; we'll presumably give it a -dev suffix if we release. we're not creating a runtime library yet I think. given that, are -dev deps okay for now?
This change would generate deps automatically ... in theory:
-set(CPACK_DEBIAN_PACKAGE_DEPENDS "libprotobuf-dev, libgrpc++-dev")
+set(CPACK_DEBIAN_PACKAGE_GENERATE_SHLIBS ON)
+set(CPACK_DEBIAN_PACKAGE_SHLIBDEPS ON)
But it detects runtime deps instead of -dev libraries:
Depends: libabsl20220623 (>= 0~20220623.0-1), libboost-log1.74.0 (>= 1.74.0+ds1), libc6 (>= 2.34), libgcc-s1 (>= 4.2), libgrpc++1.51 (>= 1.51.1), libgrpc29 (>= 1.51.1), libprotobuf32 (>= 3.21.12), libstdc++6 (>= 11)
My downstream module fails with:
/usr/include/viam/sdk/components/generic/generic.hpp:6:10: fatal error: google/protobuf/descriptor.h: No such file or directory
6 | #include <google/protobuf/descriptor.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
/usr/include/viam/sdk/common/proto_type.hpp:5:10: fatal error: boost/variant/get.hpp: No such file or directory
5 | #include <boost/variant/get.hpp>
added libboost-log-dev in 82b6883
|
||
set(CPACK_GENERATOR "DEB") | ||
set(CPACK_ARCHIVE_COMPONENT_INSTALL ON) | ||
set(CPACK_COMPONENTS_ALL viam-cpp-sdk_dev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be using CPACK_COMPONENTS_GROUPING
to get separate debs for different roles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate debs for different roles
roles = development vs runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, typically on a deb based platform, a library ships at least three packages:
- A
libfoo
package which contains the runtime role components (typically the explicitly versioned library filelibfoo.so.x.y.z
and its soname symlinklibfoo.so.x
). - A
libfoo-dev
development role package which contains the headers and the dev time lib symlinklibfoo.so
so that-lfoo
finds the library. - A
libfoo-dbg
debug role package which contains debug info.
- uses: actions/upload-artifact@v3 | ||
with: | ||
name: debian-${{ matrix.label }} | ||
path: build/viam-cpp-sdk-*.deb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the complete list of debs that gets made for a given matrix entry look like right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single .deb for viam-cpp-sdk
https://github.com/viamrobotics/viam-cpp-sdk/actions/runs/6593088962/job/17914991253#step:10:15
Uploaded /__w/viam-cpp-sdk/viam-cpp-sdk/build/viam-cpp-sdk-0.0.0-Linux.deb
.github/workflows/debian.yml
Outdated
- name: create build | ||
run: mkdir build | ||
- name: cmake | ||
working-directory: ./build | ||
run: cmake .. -G Ninja | ||
- name: build | ||
working-directory: ./build | ||
run: ninja libviamsdk.so && cpack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to use cmake
as the driver, like:
cmake -S . -B ./build -G Ninja
cmake --build ./build --target <targets> -- <ninja-options>
Also, do you need to install
before you cpack
? I'd sort of expect so, but I could be wrong. If so, thats a cmake --install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…leasing .deb packages yet
What changed
debian.yml
, which produces.deb
packages for bookworm aarch64 + x86_64Requires
line -- it adds like a minute of CPU-bound processing inside the consumer's cmake configRemaining work
Cleanup
These aren't merge blockers but should be fixed eventually: