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

Add standalone executables #216

Merged
merged 23 commits into from
May 18, 2021
Merged

Add standalone executables #216

merged 23 commits into from
May 18, 2021

Conversation

mjcarroll
Copy link
Contributor

Example of using CLI11 to build standalone executables for use with the ign tools.

This keeps the ign.cc and ign.hh the same, and just provides a C++ entrypoint.

Signed-off-by: Michael Carroll [email protected]

@mjcarroll mjcarroll self-assigned this Jan 25, 2021
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Jan 25, 2021
@mjcarroll
Copy link
Contributor Author

./bin/transport --help
transport
Usage: ./bin/transport [OPTIONS] SUBCOMMAND

Options:
  -h,--help                   Print this help message and exit
  --help-all                  Show all help
  -v,--version                

Subcommands:
  topic                       Introspect ignition topics
  service                     Introspect ignition services

@mjcarroll
Copy link
Contributor Author

./bin/transport topic --help
Introspect ignition topics
Usage: ./bin/transport topic [OPTIONS]

Options:
  -h,--help                   Print this help message and exit
  --help-all                  Show all help
  -t,--topic TEXT             Name of a topic
  -m,--msgtype TEXT           Type of message to publish
  -d,--duration FLOAT Excludes: --num
                              Duration (seconds) to run
  -n,--num INT Excludes: --duration
                              Numer of messages to echo and then exit
[Option Group: command]
  Command to be executed 
  [Exactly 1 of the following options is required]
  Options:
    -l,--list                   
    -i,--info Needs: --topic    
    -e,--echo                   
    -p,--pub TEXT Needs: --topic --msgtype

CMakeLists.txt Outdated Show resolved Hide resolved
@scpeters
Copy link
Member

requires gazebosim/gz-utils#2

@@ -0,0 +1,12 @@
add_executable(
transport
Copy link
Member

Choose a reason for hiding this comment

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

we currently embed the path of the ignition library in the cmdtransport.rb file:

I'd like to do the same, but I think this cmake target needs to be in the same scope as the code that generates the .rb file. Why not combine the cmd and ign folders? we could name it ign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with that. I'll go ahead and do that as well as update the utils target

@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Jan 26, 2021
@mjcarroll mjcarroll changed the base branch from ign-transport9 to main January 26, 2021 02:58
@scpeters
Copy link
Member

scpeters commented Feb 1, 2021

the tests were not being compiled, so I re-enabled them in 30a14ef and fixed the configuration of the cmdtransport.rb file used for testing.

I noticed that calling ign topic with any parameters other than -h or -l get treated like --list

for example:

$ ign topic -l
topic -l
/clock
/gazebo/resource_paths
/gui/camera/pose
/gui/record_video/stats
/stats
/world/default/clock
/world/default/dynamic_pose/info
/world/default/pose/info
/world/default/scene/deletion
/world/default/scene/info
/world/default/state
/world/default/stats
$ ign topic  -e -t /stats
topic -e -t /stats
/clock
/gazebo/resource_paths
/gui/camera/pose
/gui/record_video/stats
/stats
/world/default/clock
/world/default/dynamic_pose/info
/world/default/pose/info
/world/default/scene/deletion
/world/default/scene/info
/world/default/state
/world/default/stats

@scpeters
Copy link
Member

scpeters commented Feb 2, 2021

I like splitting each verb into its own executable; I've updated the ruby script to call each one in ddd1da3

there are now just two test failures, but one of them is a seg-fault in the callback function passed to CLI: https://gist.github.com/scpeters/b18b1cbb38c43e0e02d406254aaeadef

@scpeters
Copy link
Member

scpeters commented Feb 3, 2021

@osrf-jenkins run tests please

@scpeters
Copy link
Member

scpeters commented Feb 3, 2021

opened osrf/homebrew-simulation#1320 to fix brew CI

@scpeters
Copy link
Member

scpeters commented Feb 3, 2021

@osrf-jenkins run tests please

@scpeters
Copy link
Member

scpeters commented Feb 3, 2021

brew build is failing because gazebosim/gz-cmake#143 has not yet been released

@scpeters
Copy link
Member

scpeters commented Feb 6, 2021

@osrf-jenkins run tests please

@scpeters
Copy link
Member

scpeters commented Feb 6, 2021

CMakeLists.txt Outdated
@@ -74,6 +74,11 @@ else()
ign_find_package(UUID REQUIRED)
endif()

#--------------------------------------
# Find ignition-utils
ign_find_package(ignition-utils1 REQUIRED COMPONENTS cli)
Copy link
Member

Choose a reason for hiding this comment

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

I want to say we could use PRIVATE here because we aren't exporting the cli component, but if this package starts using ImplPtr or the suppression macros, then I'm not sure we'd be able to separate the core and cli components

@scpeters
Copy link
Member

scpeters commented May 3, 2021

I really can't figure out why the abichecker is complaining:

it's complaining about a bunch of removed symbols from ReqHandler and SubscriptionHandler

I believe the abichecker failure is a false positive and have noted it in gazebo-tooling/release-tools#336 (comment)

I think this is ready for review again, as I have addressed the problem noted by @caguero

@scpeters
Copy link
Member

scpeters commented May 8, 2021

I just tried echoing a topic from ign-gazebo using branch and it didn't work for me.

Listing works fine:

$ ign topic -l
/clock
/gazebo/resource_paths
/gui/camera/pose
/gui/record_video/stats
/stats
/world/default/clock
/world/default/dynamic_pose/info
/world/default/pose/info
/world/default/scene/deletion
/world/default/scene/info
/world/default/state
/world/default/stats

But echoing doesn't output anything:

$ ign topic -e -t /clock

And when I terminate the echoing with Ctrl+C I get:

^C#<Thread:0x0000559219b56e80@/usr/lib/ruby/2.5.0/open3.rb:354 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
        1: from /usr/lib/ruby/2.5.0/open3.rb:354:in `block (2 levels) in capture2e'
/usr/lib/ruby/2.5.0/open3.rb:354:in `read': stream closed in another thread (IOError)
Traceback (most recent call last):
        6: from /home/chapulina/ws_edifice/install/bin/ign:272:in `<main>'
        5: from /home/chapulina/ws_edifice/install/lib/ruby/ignition/cmdtransport10.rb:54:in `execute'
        4: from /usr/lib/ruby/2.5.0/open3.rb:349:in `capture2e'
        3: from /usr/lib/ruby/2.5.0/open3.rb:190:in `popen2e'
        2: from /usr/lib/ruby/2.5.0/open3.rb:205:in `popen_run'
        1: from /usr/lib/ruby/2.5.0/open3.rb:366:in `block in capture2e'
/usr/lib/ruby/2.5.0/open3.rb:366:in `value': Interrupt

Can someone else reproduce it?

I believe this has been fixed

Keep calling `ign topic --pub` if messages aren't received.

Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member

I noticed some flaky failures of the TopicPublish portion of UNIT_ign_TEST in the github actions runners, so I've improved the reliability of that test case in f39b7d7

@scpeters
Copy link
Member

flaky failure in GitHub action is unrelated to this PR and noted in #235 (comment)

@scpeters scpeters dismissed chapulina’s stale review May 12, 2021 17:53

it has already been fixed

@mjcarroll mjcarroll requested a review from caguero May 17, 2021 12:50
@mjcarroll
Copy link
Contributor Author

@caguero any other thoughts on this one?

Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants