Remove debian package python and pybind11 dependency #28
Conversation
paulbourelly999
changed the title
| @@ -1,13 +1,17 @@ | |||
| name: Build Workflow | |||
|
|
|||
| on: | |||
| workflow_dispatch: | |||
There was a problem hiding this comment.
Why do we need manual trigger?
There was a problem hiding this comment.
It is nice to have to be able to retrigger builds on our main branch
There was a problem hiding this comment.
If retrigger a run on existing change, can't we rerun-all-jobs in each workflow? If a new change, then the existing workflow should automatically be triggered. I am not sure I understand fully about the benefit of adding this workflow_dispatch feature.
There was a problem hiding this comment.
This repository changes relatively infrequently. When we discover issues with the pushed artifact, we often do not have the logs of the built artifact because they have expired. Having a manual way to retrigger these debian package builds allows us to more easily trace issues with the build as well as ensure that we can build this software after it's dependencies change (carma-builds) without having to make a superfluous change to trigger a build. We can not retrigger builds if nothing in the repo has changed without this manual trigger.
|
|
||
| ```python | ||
| # This path can be added via the sys module or by directly appending the PYTHON_PATH environment variable. | ||
| import sys | ||
| sys.path.append('/opt/carma/lib/') | ||
| sys.path.append('/path/to/library') |
There was a problem hiding this comment.
Which python library? What is the library path?
There was a problem hiding this comment.
Path to the library you build using the new instructions provided.
There was a problem hiding this comment.
Is this path to python library or carma-clock library?
| @@ -6,7 +6,7 @@ set(CMAKE_MODULE_PATH "$ENV{CARMA_OPT_DIR}/cmake") | |||
| set(CMAKE_CXX_STANDARD 17) | |||
|
|
|||
|
|
|||
| project(carma_clock | |||
There was a problem hiding this comment.
How this break the Cmake config file? Do we have a screenshot or issue report of this?
There was a problem hiding this comment.
It did not break the build, it broke the Cmake config so when installed other projects could not find the library even when provided the CMake config.
There was a problem hiding this comment.
Looking back at it, it is likely because changing from carma-clock to 'carma_clockalso changes the library name in cmake and so it was looking forcarma-clockConfig.cmakeinstead ofcarma_clockConfig.cmakebut updating that in all the downstream users of this library is definitely unnecessary since we have solved the problem that the-` character raised when installing this as a python module.
|
Not a blocker to PR. My only comment here is to add the description of what end-user behavior or observation triggered this change. Cause of the issues sound well documented however what symptoms did the user see that helped find these issues. |
|
@MishkaMN I added some additional description in the motivation section of this PR. Please take a look and see if that resolves your concern. |
PR Details
Description
The PR addresses several issues. The most important of which is removing Python and PyBind as a dependency from the debian package we push to our debian package repository. These dependencies were added to facilitate usage of our library by the MMITTS system which has yet to take place. They have the unintended impact that all tools currently consuming this apt package now also need python3 module and pybind11 installed and discoverable by CMake. While the build process to compile this our library into a python consumable module still works, this PR changes our build instructions to by default not include the python bindings. In the future we should consider pushing two separate debian packages (one with python bindings and one without) or determine whether we can wrap and push this as a python module.
The second fix was related to CMake config installation being broken by a recent change of the CMake project from
carma-clocktocarma_clock. This change was made initially to allow for our package to be python consumable since python does not allow-characters in module names. Changing the package name also had the unexpected result that it broke our CMake config files. After some investigation I discovered theimportlib.import_module("")functionality that allowed for importing python modules under different names which resolved the initial issue and allowed use to revert to the original cmake project name.The last change is to the CI process. Some recent efforts by GitHub to update NodeJS version used by all their runners has the unintended impact that it has some incompatibility with bionic. There is currently as flag that can be set to avoid this issue by allowing the runners to use deprecated versions of nodeJS. Ultimately we will discontinue support for bionic as soon as we remove it from CARMA Streets. I also added manual triggers for workflows and prevented our workflows from pushing debian packages during pull requests.
Related GitHub Issue
Related Jira Key
Motivation and Context
During XIL Cloud Verification testing, when attempting to rebuild CARMA Streets services with a
carma-clockdependency it was discovered that updates to this library to provide python bindings had broken build processes for including it in C++ applications already using it.The first problem was project naming. After renaming the cmake project from
carma-clocktocarma_clockto avoid invalid-characters for python modules, CMake files that were trying to include this library ascarma-clockcould not find it. We found a solution using the pythonimportlibto rename the C++ library as a python module and remove invalid characters like-from the resulting name.The second problem was that the new
carma-clocklibrary had a dependency and CMake being able to findPython3 Modulesandpybind11. CARMA Streets C++ services did not have these dependencies installed and discoverable for CMake so we have removed these dependencies. Currently we have update documentation to only allow for local building of python bindings and we plan in the future to host separate apt packages for the C++ package and the C++ package with python bindings.How Has This Been Tested?
CDASim testing
Types of changes
Checklist: