Skip to content

Require openssl so that conda verson is found before system lib#1084

Open
hlinsen wants to merge 5 commits intoNVIDIA:mainfrom
hlinsen:fix-grpc-build
Open

Require openssl so that conda verson is found before system lib#1084
hlinsen wants to merge 5 commits intoNVIDIA:mainfrom
hlinsen:fix-grpc-build

Conversation

@hlinsen
Copy link
Copy Markdown
Contributor

@hlinsen hlinsen commented Apr 9, 2026

Before this PR the system lib of openssl was picked causing potential GLIBC version mismatches on certain machines
-- Found OpenSSL: /usr/lib/x86_64-linux-gnu/libcrypto.so (found version "3.0.13")

With this PR we explicitly require openssl which forces Cmake to look for the conda path first. This
fixes unexpected compile errors.

@hlinsen hlinsen requested review from a team as code owners April 9, 2026 02:11
@hlinsen hlinsen requested a review from rgsl888prabhu April 9, 2026 02:11
@hlinsen hlinsen added bug Something isn't working non-breaking Introduces a non-breaking change labels Apr 9, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 9, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d278cb1c-8f40-44fb-9e50-f83e2438d2d0

📥 Commits

Reviewing files that changed from the base of the PR and between 9dc32ad and 1c78f98.

📒 Files selected for processing (1)
  • cpp/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
  • cpp/CMakeLists.txt

📝 Walkthrough

Walkthrough

Adds conditional OpenSSL resolution to the CMake build: if OpenSSL::SSL is absent, CMake attempts find_package(OpenSSL CONFIG QUIET) and then find_package(OpenSSL REQUIRED) before proceeding to gRPC target discovery.

Changes

Cohort / File(s) Summary
CMake Configuration
cpp/CMakeLists.txt
Insert conditional logic that, when OpenSSL::SSL is missing, runs find_package(OpenSSL CONFIG QUIET) and falls back to find_package(OpenSSL REQUIRED) prior to gRPC target resolution.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding OpenSSL requirement to prioritize conda's version over system libraries, which directly addresses the GLIBC version mismatch issue mentioned in the PR.
Description check ✅ Passed The description is directly related to the changeset, explaining the motivation (GLIBC version mismatches with system OpenSSL) and the solution (explicitly requiring OpenSSL to use conda's version).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

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 the current code and only fix it if needed.

Inline comments:
In `@cpp/CMakeLists.txt`:
- Around line 297-299: The CMake logic wrongly checks OPENSSL_FOUND but forces
config-mode with find_package(OpenSSL CONFIG REQUIRED), which breaks on systems
without OpenSSLConfig.cmake; change to a target-based config-first fallback:
first try find_package(OpenSSL CONFIG QUIET) and check for the imported target
OpenSSL::SSL (or OpenSSL::Crypto/OpenSSL::SSL), and if that target is not
present then fall back to find_package(OpenSSL MODULE REQUIRED) (or
find_package(OpenSSL REQUIRED) in module mode) so both config and module
discovery are supported; update the guard that currently references
OPENSSL_FOUND to instead test the presence of the OpenSSL::SSL target and only
call the module-mode find_package when the config-mode target is absent.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c9acaaff-128e-4ddb-bfb5-13581e6f98d9

📥 Commits

Reviewing files that changed from the base of the PR and between 378add0 and 9dc32ad.

📒 Files selected for processing (1)
  • cpp/CMakeLists.txt

@rgsl888prabhu
Copy link
Copy Markdown
Collaborator

@hlinsen you may need to update these values for wheel size change

# PyPI hard limit is 1GiB, but try to keep these as small as possible
, please check the wheel build errors.

@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot bot commented Apr 13, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@hlinsen
Copy link
Copy Markdown
Contributor Author

hlinsen commented Apr 13, 2026

/ok to test 1c78f98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants