Skip to content

Fix use of bitwise operation in CPP condition #6140

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

Merged
merged 1 commit into from
Mar 28, 2019

Conversation

bertwesarg
Copy link
Member

Signed-off-by: Bert Wesarg [email protected]

@hjelmn
Copy link
Member

hjelmn commented Dec 3, 2018

@jsquyres Do we have enough changes on master to require the next release be v5.0.0? If so we should remove the C++ bindings on master and this should go just to the releases.

@bertwesarg
Copy link
Member Author

@hjelmn @jsquyres I'm fine, if this goes only into branches, where it apply, but it is still not merged.

@jsquyres
Copy link
Member

WOW. Somehow a) I totally missed this PR (thanks for the poke, @bertwesarg), and b) this bug has been there for years.

...actually, I just did the git blame and it's only been there since b9315ed (Sep 2017). It might be worth checking which release branches this is in (v3.0.x, v3.1.x, v4.0.x).

I think we might as well fix this, even if the C++ bindings are going to go away sometime in the not-distant future (possibly Open MPI v5.0.0). If nothing else, this gives us something to cherry-pick from for the release branches.

And yes, to answer @hjelmn's question, we do have enough on master to say that it breaks ABI / backwards compatibility, so it'll be v5.0.0 (not v4.1.x.). See https://github.com/open-mpi/ompi/issues?utf8=%E2%9C%93&q=label%3A%22%F0%9F%98%B3+Backward+compat+break%22+ (although I thought there were more issues than that... OIC, I think there are more pending potential backwards compat fixes; see https://github.com/open-mpi/ompi/wiki/5.0.x-FeatureList).

I'll go ahead and merge this long-forgotten PR. @bertwesarg could you make PRs for the relevant release branches? Thanks!

@bertwesarg
Copy link
Member Author

I'll go ahead and merge this long-forgotten PR. @bertwesarg could you make PRs for the relevant release branches? Thanks!

I did, but no one merges them.

@jsquyres
Copy link
Member

jsquyres commented Apr 9, 2019

I just tagged them properly; we somehow totally missed them before, and since they weren't tagged, we continued to miss them. 😦

They missed v3.0.4 / v3.1.4 (which might even get released today), but it looks like we'll still have a v3.0.5 / v3.1.5 someday (due to a --host / --hostfile ordering issue), so we can still get this fix in the last releases on the v3.0.x and v3.1.x series.

@bertwesarg
Copy link
Member Author

I just tagged them properly; we somehow totally missed them before, and since they weren't tagged, we continued to miss them. frowning

So who should have tagged them then? I can't and all other outside contributors too. Maybe its possible to automatically set the Target labels based on the target branch? If not, maybe search by the base: [1] filter instead of these manual labels. And reinterpret the Target labels as "This merge should also go into that one" (a "Target: master" label should not be needed anymore then).

[1] https://help.github.com/en/articles/searching-issues-and-pull-requests#search-by-branch-name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants