Potential fix for CMake logic in using an external EVPath build #4697
Conversation
|
@Crivella thanks for your contributions I left a comment |
|
@Crivella I have pushed a commit to your fork that attempts to correct the tests |
vicentebolea
force-pushed
the
faeture-fix_external_evpath
branch
from
0951b70 to
3578ee3
Compare
vicentebolea
left a comment
vicentebolea
left a comment
There was a problem hiding this comment.
Thanks for the contrib
Crivella
force-pushed
the
faeture-fix_external_evpath
branch
from
3578ee3 to
6479bec
Compare
|
@Crivella is this ready to merge? |
|
I haven't dug into this yet, but what I'd wonder about is the dependencies. If using an external EVPath, that must have been built also with external FFS, DILL and ATL and we'd need to make sure we used those rather than internal. (ENET is separate and independent.) |
I was able to get it to work on top of ADIOS2/testing/adios2/unit/CMakeLists.txt Lines 11 to 13 in c60f31f I was able to get it to work either by changing those targets to use the ADIOS2/thirdparty/CMakeLists.txt Lines 137 to 141 in c60f31f to add a The other way I was able to get it to work is by changing the But I've not tested either setups on top of the internal libraries. Possibly for another Issue/PRRight now I've been testing things more with the internal libraries without UCX to get the test suite to not give a bunch of timeouts (was testing the newer version of the libraries to see if it would help but i was still hitting the timeouts) |
|
Just a note on the UCX issues. We are using that effectively in HPC environments where RDMA hardware is present. I don't think we're testing it where RDMA isn't available (I.E. where that would require UCX to fall back to TCP.). Our libfabric dataplane looks at the available transports at runtime and will report itself non-viable if there is no RDMA hardware. It looks like the UCX dataplane lacks this sort of logic. At the moment I haven't looked at UCX to see if it's possible, but that might be the answer to your UCX issues. (Or don't have UCX in the mix if there isn't RDMA available.) |
|
WRT the .sst files in the test suite. Those are indeed temporaries that are used so that the producer and consumer sides of the tests can get in touch. They are removed by the producer side during a normal shutdown, or using atexit() in the case of abnormal termination. However, there are ways for processes to die that don't allow for atexit() processing (e.g. kill -9, running under a debugger, etc.). So those files may remain and when they do they can be a problem. I run the test suite quite a bit and haven't generally run into that, but if you're finding it more common I'd be curious about the circumstances. |
|
It was mostly a problem related to how i was trying to debug the timeouts happening on the test suite. Just wanted to add it to the PR to avoid other peoples going down the same path.
Thanks for looking into this I will try to double check if it is something related to our setup
If this is the logic that needs to be enforced we might want to flip the current CMake changes to cmake_dependent_option(ADIOS2_USE_EXTERNAL_EVPATH
"Use an externally supplied EVPath library" OFF
"ADIOS2_HAVE_SST" OFF
)
cmake_dependent_option(ADIOS2_USE_EXTERNAL_ATL
"Use an externally supplied ATL library" OFF
"NOT ADIOS2_USE_EXTERNAL_EVPATH" ON
)
cmake_dependent_option(ADIOS2_USE_EXTERNAL_FFS
"Use an externally supplied FFS library" OFF
"NOT ADIOS2_USE_EXTERNAL_EVPATH" ON
)
cmake_dependent_option(ADIOS2_USE_EXTERNAL_DILL
"Use an externally supplied DILL library" OFF
"NOT ADIOS2_USE_EXTERNAL_FFS" ON
)
cmake_dependent_option(ADIOS2_USE_EXTERNAL_ENET
"Use an externally supplied ENET library" OFF
"NOT ADIOS2_USE_EXTERNAL_EVPATH" ON
)so that if |
Head branch was pushed to by a user without write access
|
The get_target_property() calls seem to be a problem when using ADIOS2_USE_EXTERNAL_EVPATH=on, but without those calls I'm not sure how to get the right include path for the sources in ADIOS2/testing/adios2/unit/... |
|
I've added my changes that i had at the time to make it work without skipping those tests d372b56 Have not test a build with the internal third-party builds with that but it seems to work up to the configure part detecting the correct paths With external deps
With internal deps
In the first case you can see al deps are taken from separate dirs and not from Can also test a quick build but i've not gotten around to try and debug the hangs/deadlocks in the test-suite yet PS: Making those find target GLOBAL will make the |
|
OK, I've tested this with external evpath builds and it all seems to work. I think the logic to try to make sure that the external evpath uses the same ffs/atl/dill that we find when searching is too much. We'll just assume that the builder is going to take care of that. @vicentebolea , this should go into master as well as the release candidate branch. I'm going to let you take care of the merge so that this happens and it doesn't get lost. |
vicentebolea
left a comment
vicentebolea
left a comment
There was a problem hiding this comment.
Thanks for the contrib!
Making those find target GLOBAL will make the XXX::XXX visible also to adjacent CMake projects, but not sure if it could mess up stuff elsewhere.
We need to use something else since GLOBAL is cmake >= 3.24 and our min supported cmake is cmake >= 3.14.
When you refer to adjacent cmake projects do you mean another cmake project within (child) of the root project of adios2?
The root project.
I think the other proposed solution of copying the needed properties from the imported targets to the internal ones might be the only way. I also had experimented with trying to use ALIAS instead of defining a new target (in |
Perhaps we can resolve this by copying more properties from the upstream target in |
|
Added the changes to copy the properties. ADIOS2/cmake/ADIOSFunctions.cmake Line 161 in ce49c36 but it would fail since not all target defines it and at some point yaml-cpp was being passed there where the target is not defined and ends up hard-failing
|
We can copy those properties if they exists in the target |
|
@Crivella please try the latest changes. |
|
That is what i had originally tried (without the if as it will just set to I went the other approach to avoid having to add an exception for every library that could not define a target using an |
|
@Crivella find the latest commit addressing this |
|
Sorry did not realize this was a
|
|
Merging since the last commit fixes the issue, we are about to release and CI passes. |
…evpath Potential fix for CMake logic in using an external EVPath build (cherry picked from commit 1a98c42)
…evpath Potential fix for CMake logic in using an external EVPath build (cherry picked from commit 1a98c42)
Merge pull request #4697 from Crivella/faeture-fix_external_evpath
* origin/release_211: Bump version to v2.11.0 Merge pull request #4697 from Crivella/faeture-fix_external_evpath ci: trim static jobs to avoid gh limits examples: do not install binaries RDMA dataplane: Share fi_fabric() and don't close until all are done (Release Branch) (#4719) Fix logic error in BP5 selective metadata aggregation (#4718) docs: add whatsnew section for 2.11 ci: quote branches to where to run the ci
3 participants
Address problem described in
Requires also the fix to
source/adios2/toolkit/remote/CMakeLists.txtin order to pick up the correct target for EVPath asEVPath::EVPathis not defined if the EVPath third-party sources are not includedNOTE: testing this on tag
2.10.2More changes are needed to find all targets on top of master (will attempt them soon) but the changes to the CMake logic still applies