Skip to content

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

Merged
wjwwood merged 1 commit intomasterfrom
revert-209-master
Jan 23, 2018
Merged

Revert "Reset RMWCount when DEALLOC rmw storage of wait set"#210
wjwwood merged 1 commit intomasterfrom
revert-209-master

Conversation

@wjwwood
Copy link
Copy Markdown
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
Copy Markdown
Member Author

wjwwood commented Jan 23, 2018

@jwang11
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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