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

Revert "Reset RMWCount when DEALLOC rmw storage of wait set" #210

Merged
merged 1 commit into from
Jan 23, 2018

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jan 23, 2018

Reverts #209

I believe this is a regression based on http://ci.ros2.org/job/ci_osx/3214/consoleFull. Since there was no CI (that I saw on the origin pr) I'll assume that's the case and revert until it can be tested separately.

@wjwwood wjwwood added the in review Waiting for review (Kanban column) label Jan 23, 2018
@wjwwood wjwwood merged commit 77321c6 into master Jan 23, 2018
@wjwwood wjwwood deleted the revert-209-master branch January 23, 2018 09:04
@wjwwood
Copy link
Member Author

wjwwood commented Jan 23, 2018

@jwang11
Copy link
Contributor

jwang11 commented Jan 23, 2018

I know what issue is, RMWCount should be passed as parameter. I'll re-submit a PR

@dirk-thomas
Copy link
Member

@wjwwood Thank you for the quick revert.

@jwang11 Please do not make any pull requests until you have compiled the patch locally and ran the tests.

@jwang11
Copy link
Contributor

jwang11 commented Jan 24, 2018

@dirk-thomas @wjwwood Sorry about that, I should have run and test it locally, but somehow neglect. I'll be more careful later on. On the other hand, it remind me that if ROS2 CI can provide a quick pre-test (ament test) mechanism for each new PR?

@wjwwood
Copy link
Member Author

wjwwood commented Jan 24, 2018

@jwang11 normally we run CI, but since it takes so long, we sometimes try to avoid it on trivial changes, but then again it bites us sometimes.

@jwang11
Copy link
Contributor

jwang11 commented Jan 24, 2018

@wjwwood Formal CI is fine, which can be triggered manually when PR is approved, like you did now. what I mean is another kind of lightweight CI test, something like "ament test compoent_path". It can be auto-triggered when new PR arrive and be done in 10 minutes.

@wjwwood
Copy link
Member Author

wjwwood commented Jan 24, 2018

Yeah, we're working on improving the CI, especially for external contributions. The problem is that if your change causes downstream changes it still bites us on our nightlies and larger changes which need "formal CI". It would help us with more trivial changes like this as a trip wire though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants