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

Install executable rqt_graph #49

Merged
merged 4 commits into from
Jul 29, 2020
Merged

Conversation

jacobperron
Copy link

This fixes a regression introduced in #42.

Now the application can be started with 'rqt_graph' or with 'ros2 run'

This fixes a regression introduced in #42.

Now the application can be started with 'rqt_graph' or with 'ros2 run'

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
setup.py Outdated
@@ -12,6 +12,7 @@
('share/' + package_name + '/resource', ['resource/RosGraph.ui']),
('share/' + package_name, ['package.xml']),
('share/' + package_name, ['plugin.xml']),
('bin', ['bin/rqt_graph']),
Copy link

@ivanpauno ivanpauno Jul 28, 2020

Choose a reason for hiding this comment

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

This is not installing an executable, but a Python script.
Thus, it's not runnable on Windows.

I'm not sure if there's a better alternative.

Copy link
Contributor

Choose a reason for hiding this comment

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

The entry point, which will result in an actual executable on Windows, needs to go to bin and the custom script to be invoked by ros2 run needs to be installed to lib/<pkgname>.

Copy link
Author

Choose a reason for hiding this comment

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

I've reverted #42 (f3d688f) so the entry point goes to bin and installed the new script to lib/rqt_graph for ros2 run to find (a6b3654).

Choose a reason for hiding this comment

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

This new approach looks better to me.
I would actually prefer to install an executable in both places, but I'm not sure if that's possible with setuptools.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually prefer to install an executable in both places

Can you explain why that would be better? (Beside the desire to have it be the same.)

I'm not sure if that's possible with setuptools.

I am not aware of a way to achieve that (excluding using custom setuptools commands for it).

Choose a reason for hiding this comment

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

Installing scripts instead of executables is sometimes problematic, e.g.: you have to use special code in launch.
See ros2/demos#374.

IMHO: We should never install scripts (neither in bin/ nor lib/<package_name>), only executables.

Copy link
Contributor

@dirk-thomas dirk-thomas Jul 28, 2020

Choose a reason for hiding this comment

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

I would argue that this is a shortcoming of launch. The command line tool ros2 run as well as launch in ROS 1 are working just fine with scripts. A package could also just provide a ruby / perl (or anything else) script and it needs to work.

(Anyway if we don't know a way to install binaries in two locations it doesn't matter.)

@ivanpauno
Copy link

Copy link
Contributor

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

This should probably target a to-be-created dashing-devel branch.

bin/rqt_graph Outdated

from rqt_graph.main import main

sys.exit(main())
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the file isn't installed to bin it should probably be in differently named directory, e.g. scripts.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, see 44266fc

We should consider making a similar change in other rqt packages that use a directory named bin (e.g. rqt_gui).

@jacobperron
Copy link
Author

This should probably target a to-be-created dashing-devel branch.

To clarify, is the reason because Crystal is EOL, or something else?

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@dirk-thomas
Copy link
Contributor

To clarify, is the reason because Crystal is EOL, or something else?

Yeah, since it is a significant change I would not risk applying it to an EOL distro.

@jacobperron jacobperron changed the base branch from crystal-devel to dashing-devel July 28, 2020 22:38
@jacobperron
Copy link
Author

jacobperron commented Jul 29, 2020

I've targeted a new dashing-devel branch and ran CI for sanity.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron merged commit 9bce79c into dashing-devel Jul 29, 2020
@jacobperron jacobperron deleted the jacob/rqt_graph_bin branch July 29, 2020 19:00
@dirk-thomas
Copy link
Contributor

dirk-thomas commented Jul 30, 2020

I've targeted a new dashing-devel branch and ran CI for sanity.

@jacobperron Please reference all related PRs / commits here to ensure the branch name was updated in all locations.

jacobperron added a commit to ros2/ros2 that referenced this pull request Jul 30, 2020
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2-gbp/rqt_graph-release that referenced this pull request Jul 30, 2020
The latest development branch is `dashing-devel` since ros-visualization/rqt_graph#49.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Author

jacobperron added a commit to ros2/ros2 that referenced this pull request Jul 30, 2020
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/ros2 that referenced this pull request Jul 30, 2020
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/ros2 that referenced this pull request Jul 30, 2020
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/ros2 that referenced this pull request Jul 30, 2020
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Author

Related PRs for remaining distros:

jacobperron added a commit to ros2-gbp/rqt_graph-release that referenced this pull request Jul 30, 2020
* Update source branch for rolling

The latest development branch is `dashing-devel` since ros-visualization/rqt_graph#49.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update branches for dashing, eloquent, and foxy

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/ros2 that referenced this pull request Jul 30, 2020
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/ros2 that referenced this pull request Jul 30, 2020
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/ros2 that referenced this pull request Jul 30, 2020
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
eholum pushed a commit to eholum/space-ros that referenced this pull request Jun 26, 2024
The dashing-devel branch was created since ros-visualization/rqt_graph#49

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
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