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

Hotfix: various build system fixes to support CI #52

Merged

Conversation

ilumsden
Copy link
Collaborator

#49 made UCX an optional dependency. However, it did not make the UCX-based code in dyad_dtl_init and dyad_dtl_finalize conditionally compiled. As a result, if a user built DYAD without --enable-ucx and then set the DTL mode to UCX, failures would occur due to dyad_dtl_init and dyad_dtl_finalize trying to call the corresponding UCX DTL functions, which would not have been built.

This hotfix PR fixes this small, but important, bug.

@ilumsden ilumsden added the bug Something isn't working label Oct 19, 2023
@ilumsden ilumsden self-assigned this Oct 19, 2023
@hariharan-devarajan
Copy link
Collaborator

Also, fix configure.ac

PKG_CHECK_VAR([UCX_LIBDIR],
    [ucx >= 1.6.0],
    [libdir],
    [],
    [AC_MSG_FAILURE([Could not find libdir for UCX])]
)
AS_IF([test "x$UCX_LIBDIR" = "x"],
    [AC_MSG_FAILURE([check_var succeeded, but value is incorrect])]
)

@hariharan-devarajan
Copy link
Collaborator

Also can u merge it with the CI PR to see if its fixed and do any other changes required for that.

@ilumsden ilumsden force-pushed the dtl_cond_compile_optional_ucx branch 4 times, most recently from 529db8e to 5d7ab91 Compare October 20, 2023 19:23
@ilumsden ilumsden changed the title Hotfix: adds conditional compilation for UCX in dyad_dtl_impl.c Hotfix: various build system fixes to support CI Oct 20, 2023
@ilumsden
Copy link
Collaborator Author

The scope of this PR has slightly changed. Now, it is a hotfix PR to address any build system bug that we encounter while testing #47.

@ilumsden ilumsden force-pushed the dtl_cond_compile_optional_ucx branch from 5d7ab91 to e86723a Compare October 24, 2023 17:04
@JaeseungYeom
Copy link
Contributor

I will wait on the update for making sure to free dtl_handle in the finalization

@ilumsden
Copy link
Collaborator Author

@JaeseungYeom I've fixed the freeing of the DTL handle in dyad_dtl_finalize. I've also rebased my branch on the newest version of main.

@ilumsden ilumsden force-pushed the dtl_cond_compile_optional_ucx branch from 871291b to 6890cd9 Compare October 30, 2023 15:10
@JaeseungYeom JaeseungYeom merged commit f26c2df into flux-framework:main Oct 31, 2023
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants