Fix the installed rl_config.hh, and delete two dead artifacts - #170
Open
mmelnich wants to merge 1 commit into
Open
Fix the installed rl_config.hh, and delete two dead artifacts#170mmelnich wants to merge 1 commit into
mmelnich wants to merge 1 commit into
Conversation
Three defects found while investigating #77. None were reachable by any build, which is why they went unnoticed. rl_config.hh does not compile, and is installed. It is the generated version header, so it is the natural place for a downstream user to look for the version macros, but including it fails with "invalid preprocessing directive #cmakedefine". Two causes, neither recent. The original version wrote the OpenMP feature flag as #CMAKEDEFINE in capitals, and configure_file is case sensitive, so that line was never substituted. The 2025 cleanup then replaced the include guard with #pragma once and removed the capitals line but left its #endif behind, and added an include of "../RandBLAS/RandBLAS/config.h.in", which is RandBLAS's CMake template rather than its generated config.h. This commit removes the bogus include and the stray #endif. The template is not repeated here because RandBLAS.hh already includes the generated config.h. RandLAPACK_HAS_OpenMP is not restored: it exists only as a CMake variable in RandLAPACKConfig.cmake.in, and the compile time flag RandLAPACK code uses is RandBLAS's. test/misc/test_config.cc is new. The header broke precisely because nothing compiled it, so the durable fix is a translation unit that does, on every platform in CI. The assertions are deliberately trivial. rl_cusolver.hh is deleted. It was a shim meant to let GPU calling code build without CUDA, by defining dummy handle types and stubs that return a nonzero cuSOLVER status. It never worked: ENABLE_CUDA was a CMake variable read by the since deleted CMake/find_cuda.cmake to decide enable_language(CUDA), and was never passed to the compiler, so its real branch was unreachable. Nothing has ever included the file. It also defines four non inline free functions in a header, so two translation units including it would collide at link time. RandLAPACK_cxx_sources is removed from RandLAPACK/CMakeLists.txt. Its only consumer was a commented out set_source_files_properties call, commented out even in the earliest commit of the surviving history. Per source compile options do nothing for an INTERFACE target with no sources; the live mechanism is target_compile_options on the interface, already present. The list was also stale, omitting linops, testing, rl_krill.hh and rl_cholqr_linops.hh. Verified against the install tree, which is where the user facing bug was: a consumer compiled with -I<prefix>/include that includes RandLAPACK/rl_config.hh now builds and reports 0.1.0-167-gab05872. rl_cusolver.hh is gone from the install tree. Suite is 789/790; the one failure, TestBQRRP.BQRRP_GPU_wide_aspect, is pre existing and fails identically on main with the same oneMKL DORGQR error.
Contributor
|
Sweet jesus the RandLAPACK install was broken. |
Contributor
Author
Yeah, and we didn't even realize! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects found while investigating #77, all in the same neighbourhood and none
reachable by any build, which is why they went unnoticed. Follow-up to #169.
1.
rl_config.hhdoes not compile, and is installedIt is the generated version header, installed into the public include tree, so it is where
a downstream user would look for the version macros. Including it fails:
Two causes, neither recent:
#CMAKEDEFINEin capitals.configure_fileis case sensitive, so that line was never substituted and the generatedheader always carried a literal, invalid directive.
RandLAPACK_HAS_OpenMPnever worked.#pragma onceand dropped thecapitals line but left its
#endiforphaned, and added#include "../RandBLAS/RandBLAS/config.h.in", which is RandBLAS's CMake template ratherthan its generated
config.h. That include is where the error above comes from.This PR removes the bogus include and the stray
#endif. The RandBLAS config is not repeatedhere because
RandBLAS.hhalready includes the generatedconfig.h.RandLAPACK_HAS_OpenMPis deliberately not restored: it exists only as a CMake variable in
RandLAPACKConfig.cmake.in,and the compile time flag RandLAPACK code actually uses is RandBLAS's.
test/misc/test_config.ccis new. The header broke precisely because nothing compiled it,so the durable fix is a translation unit that does, on every platform in CI. The assertions are
trivial on purpose; compilation is the real check.
2.
rl_cusolver.hhdeletedA shim meant to let GPU calling code build without CUDA, by defining dummy handle types and
stubs returning a nonzero cuSOLVER status so the caller's existing error checks fire. It never
worked, for two independent reasons:
ENABLE_CUDAwas a CMake variable read by the sincedeleted
CMake/find_cuda.cmaketo decideenable_language(CUDA)and was never passed to thecompiler, so
#if defined(ENABLE_CUDA)was never true; and nothing has ever included the file.Its only appearance in every commit of the visible history is inside the source list in item 3.
It also defines four non inline free functions in a header, so two translation units
including it would collide at link time.
3.
RandLAPACK_cxx_sourcesremovedIts only consumer was a commented out
set_source_files_properties(...), commented out even inthe earliest commit of the surviving history. Per source compile options do nothing for an
INTERFACEtarget with no sources; the live mechanism istarget_compile_options(RandLAPACK INTERFACE ...), already in the file. The list was also stale, omittinglinops/,testing/,rl_krill.hhandrl_cholqr_linops.hh.Verification
Checked against the install tree, since that is where the user facing bug lived:
rl_cusolver.hhconfirmed gone from the install tree. Suite is 789/790 with the two new testsincluded; the one failure,
TestBQRRP.BQRRP_GPU_wide_aspect, is pre existing and failsidentically on
mainwith the sameoneMKL ERROR: Parameter 2 was incorrect on entry to DORGQR.