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 executable #250

Closed
wants to merge 32 commits into from
Closed

Add standalone executable #250

wants to merge 32 commits into from

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 8, 2021

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

🎉 Enhancement

Summary

This PR improve support on Windows for the command line tool

Test it

ign gui
ign gui -l
ign gui -s TopicEcho
ign gui -h

Checklist

Note to maintainers: Remember to use Squash-Merge

@ahcorde ahcorde added the enhancement New feature or request label Jul 8, 2021
@ahcorde ahcorde requested a review from jennuine as a code owner July 8, 2021 15:59
@ahcorde ahcorde self-assigned this Jul 8, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Jul 8, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Jul 8, 2021
@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #250 (fbee59a) into ign-gui5 (0361d32) will increase coverage by 0.23%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui5     #250      +/-   ##
============================================
+ Coverage     66.10%   66.34%   +0.23%     
============================================
  Files            26       27       +1     
  Lines          3219     3268      +49     
============================================
+ Hits           2128     2168      +40     
- Misses         1091     1100       +9     
Impacted Files Coverage Δ
src/cmd/ign.cc 40.54% <37.50%> (ø)
src/cmd/gui_main.cc 77.27% <77.27%> (ø)
src/Application.cc 84.74% <100.00%> (+0.15%) ⬆️
src/MainWindow.cc 96.94% <100.00%> (+0.01%) ⬆️

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 0361d32...fbee59a. Read the comment docs.

@ahcorde ahcorde requested a review from mjcarroll July 8, 2021 17:29
@ahcorde ahcorde moved this from Inbox to In review in Core development Jul 8, 2021
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 12, 2021

@osrf-jenkins retest this please

@@ -598,8 +607,14 @@ std::vector<std::pair<std::string, std::vector<std::string>>>
// All we verify is that the file starts with "lib", any further
// checks would require loading the plugin.

#ifndef _WIN32
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would find it easier to read with reversed logic:

#ifdef _WIN32
      if (plugin.find(".lib") != std::string::npos)
#else
      if (plugin.find("lib") == 0)
#endif

Copy link
Member

Choose a reason for hiding this comment

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

reversed in 3b0a96b


//////////////////////////////////////////////////
extern "C" IGNITION_GUI_VISIBLE char *ignitionVersion()
Copy link
Member

Choose a reason for hiding this comment

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

I think the ABI checker is complaining that these symbols are now missing

Copy link
Member

Choose a reason for hiding this comment

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

I reverted the header file changes in 4e87255

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 13, 2021

Requires at least this PR gazebo-tooling/release-tools#487

@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 13, 2021

@osrf-jenkins retest this please

std::string cmd = std::string("IGN_CONFIG_PATH=") + ignConfigPath +
" ign gui -l";
#else
std::string ign = std::string(IGN_PATH) + "/ign.rb";
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably use path join

" && " + ign + " gui -l";
#endif
std::string output = custom_exec_str(cmd);
std::cerr << "output " << output << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this output needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope this is for testing what is going on in the CI. I will remove it when the windows CI will be happy

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
REQUIRED
PKGCONFIG "Qt5Core Qt5Quick Qt5QuickControls2 Qt5Widgets"
PKGCONFIG "Qt5Core Qt5Quick Qt5Qml Qt5QuickControls2 Qt5Widgets"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why is this needed?

@@ -598,8 +607,14 @@ std::vector<std::pair<std::string, std::vector<std::string>>>
// All we verify is that the file starts with "lib", any further
// checks would require loading the plugin.

#ifdef _WIN32
if (plugin.find(".lib") != std::string::npos)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking if you meant to use the .lib or if we should use the .dll


# Copyright (C) 2017 Open Source Robotics Foundation
# Copyright (C) 2014 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Going back in time?

src/cmd/ign.hh Outdated
@@ -18,28 +18,30 @@
#ifndef IGNITION_GUI_IGN_HH_
#define IGNITION_GUI_IGN_HH_
Copy link
Contributor

Choose a reason for hiding this comment

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

ign.hh is installed, so we need to tick-tock it.

Copy link
Member

Choose a reason for hiding this comment

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

I reverted the changes to ign.hh in 4e87255

@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 20, 2021

@osrf-jenkins retest this please

@ahcorde ahcorde force-pushed the ahcorde/5/standalone branch 3 times, most recently from 14357a1 to 1df9740 Compare July 20, 2021 17:48
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 21, 2021

@osrf-jenkins retest this please

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 22, 2021

Windows IGN_TEST is still failing. The ign-gui.exe is working properly when I compiled and ran the ign command line tool on my local machine. But in the buildfarm I'm struggling a little bit. I will post what I have tested so far.

  • Update the PATH , otherwise is not able to find some .dll.
  • I launched the build without removing the ws with this branch of release-tools, I have connected to the machine to test the WS. The test and executable is working fine.
  • I have started to thing (because of the previous point) that it's something related with the displays, I have tried to run Qt in headless mode with the env vars QT_QPA_PLATFORM_PLUGIN_PATH and QT_QPA_PLATFORM. But still, it's not working.
  • I have also included some tests (which we should remove) to test the executable without the Ruby wrapper,
    • when I used the flag --version is returning the right value, becauase this is not calling Qt
    • When I tried to list the plugins -l I get a timeout

@j-rivero do you have any idea ?

@ahcorde ahcorde moved this from In review to In progress in Core development Jul 22, 2021
Retain cmdVerbose in src/cmd/ign.hh

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 24, 2021

@osrf-jenkins retest this please

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@scpeters
Copy link
Member

@osrf-jenkins run tests please

1 similar comment
@scpeters
Copy link
Member

@osrf-jenkins run tests please

@chapulina
Copy link
Contributor

@ahcorde , are you still planning to go back to this PR? Edifice will EOL next week, how about retargeting to ign-gui6?

@j-rivero j-rivero removed this from In progress in Core development May 6, 2022
@chapulina chapulina added this to Inbox in Core development via automation Jul 29, 2022
@chapulina chapulina moved this from Inbox to In progress in Core development Jul 29, 2022
@azeey azeey added beta Targeting beta release of upcoming collection and removed beta Targeting beta release of upcoming collection labels Jul 31, 2023
@jennuine
Copy link
Contributor

jennuine commented Nov 3, 2023

@ahcorde do you plan to revisit this PR or should I close this out?

@jennuine
Copy link
Contributor

Going to close this out due to lack of movement, feel free to re-open.

@jennuine jennuine closed this Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice enhancement New feature or request
Projects
Archived in project
Core development
In progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants