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

This is an import of assert.h from rosconsole #112

Closed
wants to merge 5 commits into from
Closed

Conversation

tfoote
Copy link
Contributor

@tfoote tfoote commented Aug 6, 2018

It adds the useful macros

RCUTILS_BREAK()
RCUTILS_ASSERT(cond)
RCUTILS_ASSERT_MSG(cond, ...)
RCUTILS_ASSERT_CMD(cond, cmd)

@esteve esteve added the in progress Actively being worked on (Kanban column) label Aug 6, 2018
@tfoote tfoote added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 6, 2018
tfoote added a commit to ros2/message_filters that referenced this pull request Aug 6, 2018
Provide migration path #define and TODO for resolving once provided
upstream in ros2/rcutils#112
@mikaelarguedas
Copy link
Member

For this to keep passing the linters we will need to either update the license headers to use // (to work around ament/ament_lint#82) or disable the copyright linter for this package / file

@dirk-thomas dirk-thomas added requires-changes in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Aug 6, 2018
@tfoote
Copy link
Contributor Author

tfoote commented Aug 6, 2018

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

@tfoote tfoote added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) requires-changes labels Aug 6, 2018
@dirk-thomas
Copy link
Member

Please add some test coverage for the new header file. Currently no code uses the newly added header the CI builds won't say anything (syntax correct, behavior correct, etc.). So only the linter tests will provide some feedback. [So the jobs could have been triggered to limit the work to that package (to avoid running for so long).]

@tfoote
Copy link
Contributor Author

tfoote commented Aug 7, 2018

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

Of course there's a windows issue.

It looks like the call is correct: https://docs.microsoft.com/en-us/visualstudio/debugger/debugbreak-and-debugbreak

Rerunning:

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

@tfoote tfoote added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Aug 7, 2018
tfoote added a commit to ros2/message_filters that referenced this pull request Aug 15, 2018
Provide migration path #define and TODO for resolving once provided
upstream in ros2/rcutils#112
tfoote added a commit to ros2/message_filters that referenced this pull request Aug 15, 2018
Provide migration path #define and TODO for resolving once provided
upstream in ros2/rcutils#112
Add unit tests for assert and remove CMD version.

Signed-off-by: Tully Foote <[email protected]>
Working around ament/ament_lint#82

Signed-off-by: Tully Foote <[email protected]>
@tfoote
Copy link
Contributor Author

tfoote commented Jul 24, 2019

Rebased onto master.

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

@tfoote tfoote removed the in progress Actively being worked on (Kanban column) label Jul 24, 2019
@tfoote tfoote added the in review Waiting for review (Kanban column) label Jul 24, 2019
// CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
// LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
// ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
// POSSIBILITY OF SUCH DAMAGE.
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure importing this BSD licensed snippet is a good idea since it would affect the license of a pretty low level ROS 2 package.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can ask the authors to change the license (or dual license it, which seems to be popular these days). I didn't blame the file, but it seems likely that it's just us and Josh Faust maybe that would need to be asked?

@tfoote tfoote added backlog enhancement New feature or request and removed in review Waiting for review (Kanban column) labels Aug 7, 2019
@tfoote tfoote removed their assignment Aug 7, 2019
@tfoote
Copy link
Contributor Author

tfoote commented Jan 31, 2020

Upon further review of our licensing we already have dual licensing as low as rcpputils is a similar manner for small files imported from legacy code bases. That I identified in ros2/rcpputils#37. To that end it does not make a lot of sense to be a complete stickler for this one as opposed to that one.

If there is an issue at some point someone can reimplement it relatively easily compared to doing full due diligence and contribute the new implementation back. In the mean time this is something that would be useful for anyone migrating from ROS 1.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I'm still not convinced we couldn't just re-implement it, but if we are going to try and merge this, then the package.xml and LICENSE file needs to be updated.

@clalancette
Copy link
Contributor

This PR has been open for 4 years now, and given that there hasn't been too much movement on it, I'm going to assume that it is not all that interesting. I'm going to close it out for now, but please feel free to reopen if you think this is something we should have.

@tfoote
Copy link
Contributor Author

tfoote commented Apr 26, 2022

The ability to use the BREAK and ASSERTS with integrated messages will improve our code quality and also enable easier porting of code from ROS 1. It is a relatively small code snippet and could be re-implemented to comply with Apache 2.0 relatively easily. But I'm not sure how to do that cleanly without it being considered a derivative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants