Skip to content

Fix the installed rl_config.hh, and delete two dead artifacts - #170

Open
mmelnich wants to merge 1 commit into
mainfrom
config-header-and-dead-code
Open

Fix the installed rl_config.hh, and delete two dead artifacts#170
mmelnich wants to merge 1 commit into
mainfrom
config-header-and-dead-code

Conversation

@mmelnich

Copy link
Copy Markdown
Contributor

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.hh does not compile, and is installed

It 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:

error: invalid preprocessing directive #cmakedefine

Two causes, neither recent:

  • The original version wrote the OpenMP feature flag as #CMAKEDEFINE in capitals.
    configure_file is case sensitive, so that line was never substituted and the generated
    header always carried a literal, invalid directive. RandLAPACK_HAS_OpenMP never worked.
  • The 2025 cleanup (2025 cleanup #94) replaced the include guard with #pragma once and dropped the
    capitals line but left its #endif orphaned, and added
    #include "../RandBLAS/RandBLAS/config.h.in", which is RandBLAS's CMake template rather
    than 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 repeated
here because RandBLAS.hh already includes the generated config.h. RandLAPACK_HAS_OpenMP
is 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.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
trivial on purpose; compilation is the real check.

2. rl_cusolver.hh deleted

A 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_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 #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_sources removed

Its only consumer was a commented out set_source_files_properties(...), 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(RandLAPACK INTERFACE ...), already in the file. The list was also stale, omitting linops/, testing/,
rl_krill.hh and rl_cholqr_linops.hh.

Verification

Checked against the install tree, since that is where the user facing bug lived:

$ g++ -std=c++20 -I<prefix>/include consumer.cc -o consumer && ./consumer
0.1.0-167-gab05872 / 0.1.0

rl_cusolver.hh confirmed gone from the install tree. Suite is 789/790 with the two new tests
included; the one failure, TestBQRRP.BQRRP_GPU_wide_aspect, is pre existing and fails
identically on main with the same oneMKL ERROR: Parameter 2 was incorrect on entry to DORGQR.

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.
@rileyjmurray

Copy link
Copy Markdown
Contributor

Sweet jesus the RandLAPACK install was broken.

@mmelnich

Copy link
Copy Markdown
Contributor Author

Sweet jesus the RandLAPACK install was broken.

Yeah, and we didn't even realize!

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.

2 participants