Fix: Correct FFTW version detection and NCCL build summary - #7995
AsTonyshment wants to merge 2 commits into
Conversation
| # FFTW has no version macro; use pkg-config metadata for the selected installation. | ||
| find_package(PkgConfig QUIET) | ||
| if(PKG_CONFIG_FOUND) | ||
| pkg_check_modules(PC_FFTW3 QUIET fftw3) | ||
| get_filename_component(_fftw3_library_dir "${FFTW3_LIBRARY}" DIRECTORY) | ||
| if(PC_FFTW3_FOUND AND PC_FFTW3_INCLUDEDIR STREQUAL FFTW3_INCLUDE_DIR | ||
| AND PC_FFTW3_LIBDIR STREQUAL _fftw3_library_dir) | ||
| set(FFTW3_VERSION "${PC_FFTW3_VERSION}") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Wouldn't it duplicate the fftw3 discovery practically?
There was a problem hiding this comment.
Yeah, pkg_check_modules() indeed also performs a library lookup. If that's your concern, I've replaced it with a direct read of the Version: field from pkgconfig/fftw3.pc beside the selected library. Either way reports a valid version number on my system, while the previous logic connot.
There was a problem hiding this comment.
My initial concern is that duplicated discovery may potentially cause information inconsistency, e.g. it's possible that the fftw3 package you found from FFTW3_DIR is different from what pkg-config can see (depending on PKG_CONFIG_PATH). The current file(STRING ...) approach seems to be a compromise, but looks less clean. If I was to handle it, I would rely on pkg_check_modules() in priority to find fftw3, just like what we're currently doing for elpa.
There was a problem hiding this comment.
I would say that it's indeed a valid concern, and I do agree that using pkg-config as the primary discovery mechanism would be cleaner. However, I would not like to expand this PR into a full refactor of FFTW3 discovery. I'll keep the current compromise of reading the metadata for now. If you're interested, a separate PR to refactor the FFTW3 discovery logic would be welcome :) As “the Elder” once said, ”我说另请高明吧“!
There was a problem hiding this comment.
I'm okay with it, just explaining my concern :) I am willing to help handle it if you'd like
98170c0 to
9168bfb
Compare
2 participants
Issues
FFTW was reported as
version unknownbecause detection relied on a nonexistent header macro. NCCL parallel support was always reported asEnabled, even when disabled.Changes
pkgconfig/fftw3.pcbeside the selected library, avoiding a second dependency lookup or a dependency on the pkg-config tool. Retainversion unknownwhen the file or a literal version is unavailable.DisabledwhenENABLE_NCCL_PARALLEL_DEVICEis off.