Skip to content

threads: configury updates for argo and qthreads #10646

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
Sep 2, 2022

Conversation

hppritcha
Copy link
Member

The OAC_CHECK_PACKAGE macro doesn't have quite the same
functionality as the origina ?OPAL_CHECK_PACKAGE? macro,
so add some sed magic to get the path we need for
argo/qthread related header files.

related to #10459

Signed-off-by: Howard Pritchard [email protected]

Copy link
Contributor

@janciesko janciesko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jsquyres jsquyres requested a review from bwbarrett August 10, 2022 13:44
@janciesko
Copy link
Contributor

We're using OAC_CHECK_PACKAGE in other locations as well. Are those use-cases not affected?

@hppritcha
Copy link
Member Author

@janciesko we're doing something a little unusual in the argo/qthreads config/makefile so probably other components weren't impacted.

Copy link
Member

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're trying to accomplish here, but I'm pretty sure we don't want to do it. Can you update your commit message and add some comments so the next person doesn't have to unwind this?

@hppritcha hppritcha force-pushed the fix_item_one_of_issue10459 branch from 2977d7b to e5a50e4 Compare August 12, 2022 21:23
@hppritcha
Copy link
Member Author

I updated the changes with some text about what's going on here.

@gpaulsen gpaulsen requested a review from bwbarrett August 31, 2022 00:15
dnl extract the fully qualified include path from the opal_argo_CPPFLAGS variable
dnl for use in threads_argobots.h
AS_IF([test $opal_argo_happy = yes && test $opal_argo11_happy = yes],
[OPAL_ARGO_INCLUDE_PATH=`echo "${opal_argo_CPPFLAGS}/" | sed -e 's/\-I//g' | sed -e 's/ //g'`],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has an implicit assumption that opal_argo_CPPFLAGS only contains 1 directory, which may be true today but doesn't seem long term true.

But why are we doing this at all? It looks like we're doing it to because we need to include abt.h in a header that then is included in opal/mca/threads/threads.h. I think we'd be better off skipping this bit (that doesn't do what we test) and just adding opal_argo_CPPFLAGS into CPPFLAGS.

Accomodate changes from going from OPAL_CHECK_PACKAGET to  OAC_CHECK_PACKAGE.

related to open-mpi#10459

Signed-off-by: Howard Pritchard <[email protected]>
@hppritcha hppritcha force-pushed the fix_item_one_of_issue10459 branch from e5a50e4 to ef38d92 Compare September 1, 2022 19:29
@hppritcha
Copy link
Member Author

@janciesko i reworked things per @bwbarrett suggestions. please check that this works for you now.

Copy link
Member

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing the extra work!

@hppritcha hppritcha merged commit 2306f09 into open-mpi:main Sep 2, 2022
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.

3 participants