Skip to content

Conversation

@jacobperron
Copy link
Contributor

The README was fairly outdated. I've made updates to reflect the current state.

There's a TODO to update the section on Android. I haven't had a chance to try it out with colcon.

@jacobperron jacobperron force-pushed the jacob/update_readme branch 2 times, most recently from d85de89 to 46a6281 Compare July 1, 2020 23:30
@esteve esteve self-requested a review July 2, 2020 09:19
README.md Outdated
colcon build --symlink-install

Now you can just run a bunch of examples, head over to https://github.com/esteve/ros2_java_examples for more information.
### Download and Build ROS 2 Java for Android
Copy link
Member

Choose a reason for hiding this comment

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

I prefer not to merge this PR until the Android instructions are complete, some of the users that use ros2-java use it only because of the Android support.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd argue that this isn't making things worse for Android, it just isn't improving it either. So I'd still say we should merge this one (just so it doesn't linger), and then when somebody gets around to updating Android it would go on top of this.

Copy link
Member

@esteve esteve Jul 8, 2020

Choose a reason for hiding this comment

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

I'd argue that this isn't making things worse for Android, it just isn't improving it either.

It definitely makes things worse for Android users, because they won't have a path for building ros2_java. Currently they can build ros2_java for Bouncy, but once master only works for Dashing, they won't have instructions for building ros2_java.

and then when somebody gets around to updating Android it would go on top of this.

The goal of ros2_java is to support JRE and Android, so any contribution must target both.

Copy link
Contributor

Choose a reason for hiding this comment

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

It definitely makes things worse for Android users, because they won't have a path for building ros2_java. Currently they can build ros2_java for Bouncy, but once master only works for Dashing, they won't have instructions for building ros2_java.

I guess my main point is that this PR targets the dashing branch, which already doesn't work for Android. So we can merge it onto the dashing branch; we just can't merge the dashing branch onto master.

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 guess this raises a separate question about organization. I'd been working under the idea that we would maintain separate branches for each ROS distribution (like many other ROS packages). If users want to build on top of Bouncy, then they can refer to the bouncy branch.

To make it more clear for users, we could add a note near the top of the README referring them to the correct branch (with instructions) for their target distribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that for our use-cases we don't have as strong an interest in Android, but I do think we should still support it. Before opening this PR I did try to compile rcljava for Android to update the instructions and add CI, but haven't had any success yet. I'm not sure what needs to change in the documentation (or code) to make the instructions work. Unfortunately, since it's not a priority it's difficult for me to spend more time on it at the moment (hence the TODO). We can wait to merge this until we figure it out, but I thought it would be nicer to get the current changes in as an incremental improvement.

Regarding branches for each distro, I guess it's a matter of opinion. I think it makes maintenance and development a lot easier. Unless we only want to support the latest distro, we're bound to make changes to the default branch that breaks compatibility with older distros. If we want to maintain compatibility on a single branch, then we have to do things like "ifdef" around code blocks, which seems less pleasant (IMO) than maintaining distro-specific branches. I've tried to structure code changes such that dashing will remain compatible with Dashing and then I'd like to cherry-pick changes I have on my dev branch for Foxy compatibility to master (since the changes to Foxy are not compatibility with Dashing). I'm open to other strategies for maintaining compatibility between distros. I shouldn't have presumed that we'd use this branching method, sorry about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the end, my qualm is that OSRF has its own agenda regarding ros2-java, which seems to be aligned for the most part with the goals I have when I started this project, except for the Android stuff. But this is not an OSRF project, so all contributors are treated equally, no matter who their employer is.

Sure. That only makes sense; every contributor has their own agenda for contributing to the project.

To be honest, I'm less than enthusiastic about OSRF seemingly trying to set the goals for ros2-java, hence why I prefer to keep this project separate from them. I feel strongly about supporting Android, and any contribution must take Android into account, but you guys seem very keen on not updating ros2-java for Android in your contributions, whether code or documentation. Frankly, updating the README with instructions for Android requires much less work than setting up different branches for each distro.

We are just setting the goals for the things that we are interested in. As you can probably tell, we aren't much interested in Android. I think the disconnect we currently have is that there are 2 main branches of development:

  • Bouncy with Android support
  • Dashing and later without Android support

Jacob and I are mostly interested in the latter; you are mostly interested in the former.

It's not that we don't care at all about the Android goal of the project, but it isn't clear to me how to keep that support working. Since it isn't our main target, its hard to justify spending a lot of time setting it up.

There are several ways to rectify the disconnect:

  1. Provide a docker image to automatically build/test Android support
  2. Keep developing on separate branches, one for Android, one for other things
  3. Fork the repository

There may be others.

Copy link
Member

Choose a reason for hiding this comment

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

We are just setting the goals for the things that we are interested in.

OSRF can not set the goals for the project, basically because they are not the owners of ros2_java

It's not that we don't care at all about the Android goal of the project, but it isn't clear to me how to keep that support working. Since it isn't our main target, its hard to justify spending a lot of time setting it up.

I don't think it's that much time. I'm more than happy to help you if you have problems with setting it up

I think the disconnect we currently have is that there are 2 main branches of development:

Bouncy with Android support
Dashing and later without Android support

Maintaining multiple branches requires a lot of effort, which in this case is completely unnecessary because there's nothing in the code that prevents both platforms from being supported. My goal is to only have one branch, it's much simpler that way

Keep developing on separate branches, one for Android, one for other things

The Android code is isolated in other layers in ROS 2, there's nothing in ros2_java, either in master or the dashing branch that prevents it from running on Android, only the instructions need to be updated.

Fork the repository

Whoa, the nuclear option just because you don't want to update a README. Frankly, not a big fan of this level of confrontation

There may be others.

When contributing to a project you are not even a committer, it's good etiquette to try to cooperate at least, at least from my experience

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've pushed changes to the instructions that I've tried so far and opened a PR targeting this branch #125

I can continue to chip away at it in some free time, but any insights would be greatly appreciated 🙂

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've merged #125 here, containing instructions for Android (and CI!) (a3fa46d)

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Currently, we only have CI for Dashing on Linux.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
For more granularity in header levels.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
* Update Android instructions

This is a work in progress, still troubleshooting build issues.

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

* Update Android repos file

Now the repos file contains all packages in the upstream dashing repos file, without the Qt packages:

https://github.com/ros2/ros2/blob/dashing/ros2.repos
Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Added job for Android

* Only build the rcljava subset of dependencies

* Fix URL

* Replace ros-tooling scripts with plain commands

* Fix typo

* Install vcstool for vcs

* Install Android NDK

* Add ros2_java workspace to CMake find root path

* Disable building tests

* Do not build osrf_testing_tools_cpp as it uses execinfo.h, which is not available on Android

* Workaround for Android

* Reenable building tests

* Skip cyclonedds for now

* Install lark parser

* Install python3-dev

* Pass PYTHON_LIBRARY and PYTHON_INCLUDE_DIR to the CMake Python module

* Skip rcl_logging_log4cxx

* Use rmw fork for Android. Skip CycloneDDS packages

* Revert to 1.6

* Configure log4j dynamically to avoid errors when building for Android

* Configure log4j dynamically to avoid errors when building for Android

* Switch to jacob/instructions temporarily

* Fix compilation error

* Trim repos file for Android

* Readd googletest repo

* Fix missing import

* Build entire workspace

* Only build up to the Android examples

* Install colcon extensions for Gradle

* Revert changes for desktop

* Move ros2_android repo to ros2-java org

* Install Gradle

* Build rcljava_examples

* Install Gradle

* Do not build rcljava_examples temporarily

* Update README.md

* Do not use fork

* Use repos files from GitHub workspace

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

* Add checkout action for cloning the branch to test

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

* Use version of ros2_java in the branch being tested

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

* Switch back to dashing branch

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

Co-authored-by: Esteve Fernandez <esteve@apache.org>
@jacobperron jacobperron requested a review from esteve November 19, 2020 00:08
@jacobperron
Copy link
Contributor Author

@esteve Friendly ping; I think all comments in the PR have been addressed. The only outstanding issue is #137. Let me know if you think this is okay to merge to dashing.

@jacobperron
Copy link
Contributor Author

I'm going ahead to merge this to dashing to take advantage of the CI changes.

I'm happy to address any more feedback in subsequent PRs.

@jacobperron jacobperron merged commit efe2df5 into dashing Dec 4, 2020
@jacobperron jacobperron deleted the jacob/update_readme branch December 4, 2020 01:08
jacobperron added a commit to osrf/ros2_java that referenced this pull request Dec 8, 2020
* Cherry-pick upstream ros2-java#119
  - Updates build instructions in README
  - Update CI for desktop
  - Add CI for Android
  - Minor patches to code for Android

* Switch to latest release of ros-tooling GitHub actions.
jacobperron added a commit to osrf/ros2_java that referenced this pull request Jan 5, 2021
* Cherry-pick upstream ros2-java#119
  - Updates build instructions in README
  - Update CI for desktop
  - Add CI for Android
  - Minor patches to code for Android

* Switch to latest release of ros-tooling GitHub actions.
jacobperron added a commit that referenced this pull request May 17, 2021
* Use curl for consistency

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

* Update build badges

Currently, we only have CI for Dashing on Linux.

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

* Update URLs

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

* Minor formatting changes

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

* Update build instructions for desktop

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

* Use different header markdown syntax

For more granularity in header levels.

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

* Fix whitespace formatting

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

* Formatting

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

* Add ref to ticket for ROS time

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

* 'merge install' for Windows

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

* Fix typo

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

* Update Android instructions with CI (#125)

* Update Android instructions

This is a work in progress, still troubleshooting build issues.

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

* Update Android repos file

Now the repos file contains all packages in the upstream dashing repos file, without the Qt packages:

https://github.com/ros2/ros2/blob/dashing/ros2.repos
Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Added job for Android

* Only build the rcljava subset of dependencies

* Fix URL

* Replace ros-tooling scripts with plain commands

* Fix typo

* Install vcstool for vcs

* Install Android NDK

* Add ros2_java workspace to CMake find root path

* Disable building tests

* Do not build osrf_testing_tools_cpp as it uses execinfo.h, which is not available on Android

* Workaround for Android

* Reenable building tests

* Skip cyclonedds for now

* Install lark parser

* Install python3-dev

* Pass PYTHON_LIBRARY and PYTHON_INCLUDE_DIR to the CMake Python module

* Skip rcl_logging_log4cxx

* Use rmw fork for Android. Skip CycloneDDS packages

* Revert to 1.6

* Configure log4j dynamically to avoid errors when building for Android

* Configure log4j dynamically to avoid errors when building for Android

* Switch to jacob/instructions temporarily

* Fix compilation error

* Trim repos file for Android

* Readd googletest repo

* Fix missing import

* Build entire workspace

* Only build up to the Android examples

* Install colcon extensions for Gradle

* Revert changes for desktop

* Move ros2_android repo to ros2-java org

* Install Gradle

* Build rcljava_examples

* Install Gradle

* Do not build rcljava_examples temporarily

* Update README.md

* Do not use fork

* Use repos files from GitHub workspace

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

* Add checkout action for cloning the branch to test

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

* Use version of ros2_java in the branch being tested

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

* Switch back to dashing branch

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

Co-authored-by: Esteve Fernandez <esteve@apache.org>

Co-authored-by: Esteve Fernandez <esteve@apache.org>
ivanpauno pushed a commit to osrf/ros2_java that referenced this pull request May 17, 2021
* Cherry-pick upstream ros2-java#119
  - Updates build instructions in README
  - Update CI for desktop
  - Add CI for Android
  - Minor patches to code for Android

* Switch to latest release of ros-tooling GitHub actions.
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.

4 participants