Skip to content

fix(cmake): Correctly report system BLAS in summary #7230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 26, 2025

Conversation

sincethestudy
Copy link
Contributor

fix(cmake): Correctly report system BLAS in configuration summary

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

When configuring Open3D with the USE_SYSTEM_BLAS=ON option, the final configuration summary table incorrectly reported BLAS ..... no. This occurred even though CMake successfully found the system BLAS/LAPACK libraries and configured the build to use them (verified via CMakeCache.txt and the addition of the USE_BLAS preprocessor definition).

This misleading summary output could cause confusion for users building with system BLAS. This change corrects the summary output to accurately reflect the configuration.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test results (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

This PR modifies the cmake/Open3DPrintConfigurationSummary.cmake script.

The original logic for printing the status of third-party dependencies checked for the existence of an internal CMake target (Open3D::3rdparty_${dep_lower}). However, when USE_SYSTEM_BLAS=ON, this target (Open3D::3rdparty_blas) is apparently not created, causing the script to fall into the else block and print "no".

The fix adds a specific condition within this else block: if the dependency being checked is "BLAS" and the USE_SYSTEM_BLAS variable is ON, it now correctly prints "yes (system)" instead of "no". This ensures the summary accurately reflects the use of a system-provided BLAS library.

Before:
-- Third-Party Dependencies: -- BLAS .................................... no
After:
-- Third-Party Dependencies: -- BLAS .................................... yes (system)

Copy link

update-docs bot commented Apr 25, 2025

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@sincethestudy sincethestudy force-pushed the fix/summary-system-blas branch from ad57d25 to 4513697 Compare April 25, 2025 17:33
@ssheorey ssheorey requested a review from Copilot May 23, 2025 19:12
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the CMake summary table so that enabling USE_SYSTEM_BLAS shows “yes (system)” instead of “no” and documents the change in the changelog.

  • Correct CMake logic to report system BLAS in the configuration summary.
  • Add an entry to CHANGELOG.md.
Files not reviewed (1)
  • cmake/Open3DPrintConfigurationSummary.cmake: Language not supported

CHANGELOG.md Outdated
@@ -56,7 +56,7 @@
- Fix render to depth image on Apple Retina displays (PR #7001)
- Fix infinite loop in segment_plane if num_points < ransac_n (PR #7032)
- Add select_by_index method to Feature class (PR #7039)

- Fix CMake configuration summary incorrectly reporting `no` for system BLAS.
Copy link
Preview

Copilot AI May 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Remove the empty bullet entry at line 58 to avoid an extraneous blank item in the changelog list.

Copilot uses AI. Check for mistakes.

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

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

Thanks @sincethestudy !

@ssheorey ssheorey merged commit e4c3b38 into isl-org:main May 26, 2025
33 of 39 checks passed
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