Consolidated: fix OFI configury / linking issues #6363
Conversation
|
@rashikakheria Could you please try this PR and see if it resolves the issue for you? |
|
Because that lets us run the OFI checks just once (vs. run the tests again for each component). |
|
From |
|
I somewhat prefer that the results end up in a documented single set of variables instead of 3 sets of variables (one each for btl/ofi, btl/usnic, and mtl/ofi). But I don't feel strongly about this. I know that other components do it the other way -- but this morning when I was coding it up:
But again, I don't have strong feelings either way. If you'd prefer that I put it back to having the output names reflect the components, I can do that. Lemme know. 😄 |
I can confirm that this fixes my linking issue. Thanks for all of your assistance! |
|
@rashikakheria Awesome; thanks. I think @ggouaillardet and I might iterate a little more to get the solution in its final state (he's many timezones away / asleep right now), but we're darn close. |
|
@jsquyres, I did effectively start looking at this but got pulled by other stuff (apologize). In any case, I also tested this PR and is working for me. thanks, |
|
@jsquyres for maintainability purpose, I think @bwbarrett do you want to comment on that PR ? |
|
@ggouaillardet Ah -- there's an easy way to fix that: merge #6270 (I had forgotten that it wasn't already merged! 😜) . That gets rid of all the verbs stuff, and all we're left with is the "new" style libfabric stuff. I've just rebased #6270; I'll merge it when I finish a few sanity tests and CI completes. |
jsquyres
force-pushed
the
pr/fix-ofi-configury
branch
from
f37efe8 to
39e0d54
Compare
Signed-off-by: Gilles Gouaillardet <[email protected]>
Replace all tabs with spaces. No code or logic changes. Signed-off-by: Jeff Squyres <[email protected]>
Replace all tabs with spaces. No code or logic changes. Signed-off-by: Jeff Squyres <[email protected]>
Update the OPAL_CHECK_OFI configury macro:
- Make it safe to call the macro multiple times:
- The checks only execute the first time it is invoked
- Subsequent invocations, it just emits a friendly "checking..."
message so that configure output is sensible/logical
- With the goal of ultimately removing opal/mca/common/ofi, rename the
output variables from OPAL_CHECK_OFI to be
opal_ofi_{happy|CPPFLAGS|LDFLAGS|LIBS}.
- Update btl/ofi, btl/usnic, and mtl/ofi for these new conventions.
- Also, don't use AC_REQUIRE to invoke OPAL_CHECK_OFI because that
causes the macro to be invoked at a fairly random time, which makes
configure stdout confusing / hard to grok.
- Remove a little left-over kruft in OPAL_CHECK_OFI, too (which
resulted in an indenting change, making the change to
opal_check_ofi.m4 look larger than it really is).
Thanks Alastair McKinstry for the report and initial fix.
Thanks Rashika Kheria for the reminder.
Signed-off-by: Jeff Squyres <[email protected]>
It never lived up to its purpose (and has caused amorphous indirect errors such as open-mpi#2519), so delete it. Signed-off-by: Jeff Squyres <[email protected]>
jsquyres
force-pushed
the
pr/fix-ofi-configury
branch
from
39e0d54 to
dd20174
Compare
|
I rebased this on the HEAD of master, just for good measure. After CI finishes and this PR is merged:
@matcabral I've sunk a lot of time in this already -- can I ask you to make the v3.0.x PR? (i.e., cherry pick just those 2 commits and fix all the conflicts) |
|
@jsquyres, sure, I'll look at 3.0.x, either today or tomorrow. |
This PR is a consolidation of several existing PRs -- this is simpler than having a bunch of individual PRs that may end up having merge ordering issues:
OPAL_CHECK_OFImacroAlso includes some commits to remove pesky tabs. Down with tabs!
FYI @rashikakheria