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

Support backward-cpp #2

Merged
merged 9 commits into from
Oct 26, 2021
Merged

Support backward-cpp #2

merged 9 commits into from
Oct 26, 2021

Conversation

j-rivero
Copy link
Contributor

Related to gazebosim/gz-tools#63

The PR makes several things:

  • remove Sid. ignition-tools is an official package in Sid started last week
  • backward-cpp is available in Debian/Buster and Ubuntu/Focal on but not on Bionic. Missed the build-dep on control file, fixed here.
  • For BIonic: custom rules and control file to use the vendor backward-cpp.

@@ -6,7 +6,8 @@ Priority: optional
Build-Depends: debhelper (>= 9~),
gem2deb,
cmake,
pkg-config
pkg-config,
libbackward-cpp-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

This kinda goes against the undocumented rule we have against adding dependencies in minor releases. In this case I guess the alternative would be vendoring, which I don't think is recommended.

Do you think the addition of this dependency could negatively impact downstream users? Note that ign-tools1 is a dependency of pretty much all simulators we maintain right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think the addition of this dependency could negatively impact downstream users? Note that ign-tools1 is a dependency of pretty much all simulators we maintain right now.

This case is a bit special since the dependency is only a build-dep, headers go into the build and no more is needed, if I understand the scenario correctly. If this is right, we are not modifying the dependency chain for consumers of the library just altering the build conditions providing the necessary source code for platforms without backwards-cpp. Seems safe to me to make an exception to the rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, I agree that it's ok if it's only needed while building. To be honest, I haven't looked into how it's all hooked up. I just noticed some failing downstream CI that couldn't find backward though.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Let's give it a try!

@j-rivero
Copy link
Contributor Author

Double checking things, seems like the new code does produce a new library:

++ cat /var/lib/jenkins/workspace/ignition_tools-ci-ign-tools1-bionic-amd64/make_jobs
+ make -j4
Scanning dependencies of target backward
[ 50%] Building CXX object src/CMakeFiles/backward.dir/backward.cc.o
[100%] Linking CXX shared library libignition-tools-backward.so
[100%] Built target backward
+ echo '# END SECTION'
# END SECTION
+ echo '# BEGIN SECTION: installing'

I assume that the library is needed to use the new functionality, is this right @azeey? If it is, we need a new package (i.e: libignition-tools-backwards) created from here and more modifications to this PR.

@azeey
Copy link
Contributor

azeey commented Oct 19, 2021

I assume that the library is needed to use the new functionality

That's correct.

@chapulina
Copy link
Contributor

In that case, is there a way to setup the core ign-tools to skip backwards utilization if it's not present at runtime? So we keep the core of ign-tools C++-free and don't add a new dependency to existing users.

@azeey
Copy link
Contributor

azeey commented Oct 19, 2021

That's what it does right now. It tries to load the library and prints a message if it fails https://github.com/ignitionrobotics/ign-tools/blob/6cf3e3521f701d79f1c1e11b2b6d9e3de7f946fe/src/ign.in#L292-L297.

Seeing that Backward was a header-only library, I was hoping we can just drop it in the code and not have to deal with packaging issues. I guess it's not that simple.

@j-rivero
Copy link
Contributor Author

That's what it does right now. It tries to load the library and prints a message if it fails https://github.com/ignitionrobotics/ign-tools/blob/6cf3e3521f701d79f1c1e11b2b6d9e3de7f946fe/src/ign.in#L292-L297.

Maybeeee a warning? not under the scope of this work, just a comment.

Seeing that Backward was a header-only library, I was hoping we can just drop it in the code and not have to deal with packaging issues. I guess it's not that simple.

That usually happens when we have C/C++ libraries and header only libraries are introduced in the code and just mixed at compilation time with no other requirements or effects. In this case, the library has no C/C++ code so somehow the feature needs a place to live in :)

I'm including the library together with the main package 2e6dc66. Let me role a prerelease to see if things are working.

@j-rivero
Copy link
Contributor Author

I'm including the library together with the main package 2e6dc66. Let me role a prerelease to see if things are working.

1.4.0 Focal/amd64 from prerelease repository:

❯ ign log
Record and playback Ignition Transport topics.                        

  ign log record|playback [options]                                     
                                                                        
Options:                                                              

  -h [ --help ]              Print this help message.                   
  -v [ --verbose ] LEVEL     Set verbosity level 0-4 (default 1).       
                                                                        

~/code 
❯ sudo rm /usr/lib/x86_64-linux-gnu/libignition-tools-backward.so

~/code 
❯ ign log
Library error: libignition-tools-backward.so not found. Improved backtrace generation will be disabled
Record and playback Ignition Transport topics.                        

  ign log record|playback [options]                                     
                                                                        
Options:                                                              

  -h [ --help ]              Print this help message.                   
  -v [ --verbose ] LEVEL     Set verbosity level 0-4 (default 1).       
                                                                       

@j-rivero j-rivero closed this Oct 26, 2021
@j-rivero j-rivero reopened this Oct 26, 2021
@j-rivero j-rivero merged commit 1fa21cd into main Oct 26, 2021
@j-rivero j-rivero deleted the backward_cpp branch October 26, 2021 17:12
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.

3 participants