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

Added standalone executable #65

Merged
merged 7 commits into from
Nov 17, 2021
Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Nov 3, 2021

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

Depends on #64

Related with this issue #54

Summary

Added standalone executable. This should improve Windows support.

There is no tests, I need to add them

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from mxgrey as a code owner November 3, 2021 11:09
@ahcorde ahcorde self-assigned this Nov 3, 2021
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #65 (6e184ae) into main (ab03ff3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #65   +/-   ##
=======================================
  Coverage   99.82%   99.83%           
=======================================
  Files          15       16    +1     
  Lines         584      615   +31     
=======================================
+ Hits          583      614   +31     
  Misses          1        1           
Impacted Files Coverage Δ
loader/src/cmd/ign.cc 96.00% <100.00%> (ø)
loader/src/cmd/plugin_main.cc 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab03ff3...6e184ae. Read the comment docs.

Base automatically changed from ahcorde/1_to_main_8_3_2021 to main November 3, 2021 18:47
@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 3, 2021

@scpeters, I will open another PR to enable windows UNIT_IGN_TEST

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

made a few minor comments; this looks really good

there's one kinda confusing part in the help string generated by cli11, so I opened a feature request in ign-utils about improving the formatting ( gazebosim/gz-utils#28 ), but it's not blocking this PR for me

loader/src/cmd/cmdplugin.rb.in Outdated Show resolved Hide resolved
loader/src/cmd/CMakeLists.txt Outdated Show resolved Hide resolved
loader/src/cmd/plugin_main.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@scpeters
Copy link
Member

scpeters commented Nov 4, 2021

to improve coverage, perhaps we need to add to UNIT_ign_TEST so that it calls ign plugin or ign plugin --help?

@@ -29,7 +52,7 @@ set(cmd_script_configured "${CMAKE_CURRENT_BINARY_DIR}/cmd${IGN_DESIGNATION}${PR

# Set the library_location variable to the relative path to the library file
# within the install directory structure.
set(library_location "../../../${CMAKE_INSTALL_LIBDIR}/$<TARGET_FILE_NAME:${loader}>")
set(plugin_exe_location "${CMAKE_INSTALL_PREFIX}/${INSTALL_STANDALONE_EXECUTABLE}/$<TARGET_FILE_NAME:${plugin_executable}>")
Copy link
Member

Choose a reason for hiding this comment

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

actually, I think we want to keep it as a relative path so that the ruby file is relocatable. The ../../.. is to back out from lib/ruby/ignition (see line 67)

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from scpeters November 15, 2021 16:13
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@ahcorde ahcorde merged commit 7c81bcb into main Nov 17, 2021
@ahcorde ahcorde deleted the ahcorde/main/standalone_executable branch November 17, 2021 20:07
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.

2 participants