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

CMAKE: Make CMake config idempotent w.r.t. target discovery and creation #9622

Closed
wants to merge 1 commit into from

Conversation

vyasr
Copy link

@vyasr vyasr commented Jan 22, 2024

What

This PR makes it safe to call find_package(ucx) multiple times in the same scope in CMake.

Resolves #9614

Why ?

In complex build environments where multiple packages may depend on ucx, it may be found twice in different parts of the CMake code. Depending on the relative scope of these find_package calls, calling find_package(ucx) multiple times in the same or nested scopes is a common occurrence.

How ?

This PR changes the ucx-targets.cmake file to check which targets are defined before trying to create new ones. It exits early if all the desired targets have already been created in this scope.

@vyasr vyasr changed the title Make CMake config idempotent w.r.t. target discovery and creation CMAKE: Make CMake config idempotent w.r.t. target discovery and creation Jan 22, 2024
@pentschev
Copy link
Contributor

@vyasr could you squash commits? I think in general only one commit per PR is expected, the first one seems to be named correctly but the latest isn't, squashing them into a single commit should resolve that and be in line with what is normally expected in this repo.

@vyasr
Copy link
Author

vyasr commented Jan 29, 2024

@vyasr could you squash commits? I think in general only one commit per PR is expected, the first one seems to be named correctly but the latest isn't, squashing them into a single commit should resolve that and be in line with what is normally expected in this repo.

Done.

@vyasr
Copy link
Author

vyasr commented Jan 30, 2024

I don't think the one failure is related, but I don't know ucx well enough to be sure. It seems like an unrelated build issue on the runner though.

@pentschev
Copy link
Contributor

I don't think the one failure is related, but I don't know ucx well enough to be sure. It seems like an unrelated build issue on the runner though.

I agree, the failure is here and seems to be related to mlx5/verbs:

Check for ./uct_info_static
tcp... ok
self... ok
sysv... ok
posix... ok
dc_mlx5... fail
rc_mlx5... fail
ud_mlx5... fail
rc_verbs... fail
ud_verbs... fail
cma... ok
failed to check tl in ./uct_info_static
##[error]Bash exited with code '1'.

@yosefe @brminich could you please confirm the above isn't related? I think this is ready for review too, could you please take a look when you have the chance?

@tvegas1
Copy link
Contributor

tvegas1 commented Feb 20, 2024

merged as #9696

@tvegas1 tvegas1 closed this Feb 20, 2024
@vyasr vyasr deleted the fix/cmake_targets branch February 22, 2024 16:35
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.

CMake fails if find_package(ucx) happens more than once
4 participants