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

Fix typo in liboshmem name #221

Merged
merged 1 commit into from Oct 6, 2014
Merged

Fix typo in liboshmem name #221

merged 1 commit into from Oct 6, 2014

Conversation

opoplawski
Copy link
Contributor

No description provided.

@jsquyres
Copy link
Member

jsquyres commented Oct 3, 2014

@miked-mellanox @jladd-mlnx This looks like a trivial / obvious fix.

My question is: how did this work at all with this typo? Are we missing something obvious here?

@opoplawski
Copy link
Contributor Author

I think all it affected was the determination of whether the shared library was available. Actual link flags are in the libs* -l settings.

@jsquyres
Copy link
Member

jsquyres commented Oct 3, 2014

But it still should have always complained that the library wasn't there, right?

@opoplawski
Copy link
Contributor Author

Possibly, but at the moment the logic in opal_wrapper.c is:

            if (have_dyn_lib) {
                use_static_libs = false;
            } else {
                use_static_libs = true;
            }

So it blindly uses the static link line if the shared library isn't found. This is how I found the issue.

@jsquyres
Copy link
Member

jsquyres commented Oct 3, 2014

Ah, ok. That makes sense.

Even though it looks trivial/correct to me, I'd still like @miked-mellanox and @jladd-mlnx to ack this before we accept it (it's their code, after all).

@jsquyres jsquyres closed this Oct 3, 2014
@mike-dubman
Copy link
Member

looks right.

@jladd-mlnx
Copy link
Member

Ack. Good catch.

@jsquyres jsquyres reopened this Oct 6, 2014
@jsquyres
Copy link
Member

jsquyres commented Oct 6, 2014

Yikes; looks like I pushed the wrong button a few days ago ("close and comment" vs. "comment").

Ok, Mike and Josh like it, so let's do it.

jsquyres added a commit that referenced this pull request Oct 6, 2014
Fix typo in liboshmem name
@jsquyres jsquyres merged commit cd48fbe into open-mpi:master Oct 6, 2014
@opoplawski
Copy link
Contributor Author

It would be nice to have this ported back to 1.8.X as well.

@rhc54
Copy link
Contributor

rhc54 commented Apr 6, 2015

Done - thx for pointing it out

jsquyres pushed a commit to jsquyres/ompi that referenced this pull request Sep 21, 2016
Don't error out with default bindings if binding isn't supported
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.

None yet

5 participants