Skip to content

issue: 5148137 Use tls detection flags in tls.m4 link-test - #249

Open
pasis wants to merge 1 commit into
Mellanox:sockperf_v2from
pasis:bugfix/tls-m4-detection-flags
Open

issue: 5148137 Use tls detection flags in tls.m4 link-test#249
pasis wants to merge 1 commit into
Mellanox:sockperf_v2from
pasis:bugfix/tls-m4-detection-flags

Conversation

@pasis

@pasis pasis commented Jul 12, 2026

Copy link
Copy Markdown
Member

The TLS support link-test applied $sockperf_cv_dpcp_LDFLAGS and $sockperf_cv_dpcp_CXXFLAGS, which are never set in this macro, instead of the $sockperf_cv_tls_* variables assigned just above. With OpenSSL in a non-default prefix the detection link-test could not find -lssl, so detection failed and TLS was silently disabled
(checking for tls support... no).

Use $sockperf_cv_tls_LDFLAGS so the OpenSSL -L/-rpath derived from --with-tls= reach the link-test.

Remove CXXFLAGS usage, because it's unused in the C mode check.

Hard fail the configure stage if --with-tls requested and openssl is not found or the link test fails.

Summary by CodeRabbit

  • Bug Fixes
    • Improved TLS-enabled builds by applying the correct TLS-specific compiler and linker flags during configuration.
    • Adjusted detection behavior so TLS library/header checks are handled more reliably.
    • When TLS is explicitly requested and isn’t usable, configuration now fails with a clear error instead of proceeding with a “no TLS” outcome.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The TLS configuration macro changes probe flag handling, applies only TLS-specific linker flags when TLS is enabled, adjusts the OpenSSL header check, and aborts configuration when explicitly requested TLS is unavailable.

Changes

TLS Build Configuration

Layer / File(s) Summary
Wire TLS probe and build flags
config/m4/tls.m4
TLS probing no longer saves CXXFLAGS; enabled TLS configuration prepends sockperf_cv_tls_LDFLAGS without the prior DPCP flag injection.
Validate TLS availability
config/m4/tls.m4
The OpenSSL header check changes failure handling, and explicit TLS requests now terminate configuration when TLS is unavailable.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Poem

I’m a rabbit with TLS in my burrow,
Flags now follow the proper furrow.
Headers test, and errors speak,
Requested TLS won’t hide or sneak.
Hop, hop—configuration’s clear!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is specific and matches the main change: using TLS detection flags in the tls.m4 link test.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@config/m4/tls.m4`:
- Line 48: In TLS_CAPABILITY_SETUP, remove the CXXFLAGS assignment referencing
sockperf_cv_tls_CXXFLAGS, since the variable is not initialized and the probe
runs in C mode; retain the existing CPPFLAGS handling for the TLS include path.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Enterprise

Run ID: 925dc02a-33e2-4886-83f5-6913e9f43b1d

📥 Commits

Reviewing files that changed from the base of the PR and between 3c65ad9 and 48403cf.

📒 Files selected for processing (1)
  • config/m4/tls.m4

Comment thread config/m4/tls.m4 Outdated
@BasharRadya

Copy link
Copy Markdown

Consider a configure warning when --with-tls= is explicitly set but detection fails, instead of silently falling back to "no".

Comment thread config/m4/tls.m4 Outdated
The TLS support link-test applied $sockperf_cv_dpcp_LDFLAGS and
$sockperf_cv_dpcp_CXXFLAGS, which are never set in this macro, instead
of the $sockperf_cv_tls_* variables assigned just above. With OpenSSL
in a non-default prefix the detection link-test could not find -lssl,
so detection failed and TLS was silently disabled
(checking for tls support... no).

Use $sockperf_cv_tls_LDFLAGS so the OpenSSL -L/-rpath derived from
--with-tls=<prefix> reach the link-test.

Remove CXXFLAGS usage, because it's unused in the C mode check.

Hard fail the configure stage if --with-tls requested and openssl is
not found or the link test fails.

Signed-off-by: Dmytro Podgornyi <dmytrop@nvidia.com>
@pasis
pasis force-pushed the bugfix/tls-m4-detection-flags branch from 48403cf to 4c66538 Compare July 13, 2026 06:36
@pasis

pasis commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

Consider a configure warning when --with-tls= is explicitly set but detection fails, instead of silently falling back to "no".

done.

@pasis

pasis commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

bot:retest

1 similar comment
@pasis

pasis commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

bot:retest

@igor-ivanov

Copy link
Copy Markdown
Collaborator

bot:retest

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.

3 participants