Use the default/cached value when a BuildConfig field is None

[wujingyue]

Fixes #5824

[wujingyue]

!test

[github-actions]

Description

• Make BuildConfig fields optional (bool | None) to allow default/cached values

• Update cmake command generation to only set variables when field is not None

• Fix cpp_standard check to handle None values safely

• Improve build_dir logic to use default when empty

Changes walkthrough

	Relevant files
Bug fix	setup.py Fix cpp_standard None check                                                            python/setup.py Add None check for config.cpp_standard before comparison Prevents TypeError when cpp_standard field is None +1/-1
Enhancement	utils.py Make BuildConfig fields optional and update cmake generation python/utils.py Make BuildConfig fields optional (bool | None) instead of required defaults Refactor cmake command to conditionally set variables only when not None Update build_dir logic to use default when empty string Add documentation about None handling for environment variables +53/-37

PR Reviewer Guide

Here are some key observations to aid the review process:

🧪 No relevant tests

⚡ Recommended focus areas for review

Ninja Generator Logic Error The condition if config.no_ninja is not None and not config.no_ninja: on line 328 is incorrect. When no_ninja is None (default), this evaluates to False, so Ninja generator is not added. However, the old code if not config.no_ninja: would add Ninja when no_ninja was None or False. This changes the default behavior - previously Ninja was used by default when no_ninja was not explicitly set to True, but now Ninja is only used when no_ninja is explicitly set to False. This could break existing builds that relied on the default Ninja generator. if config.no_ninja is not None and not config.no_ninja: cmd_str.append("-G") cmd_str.append("Ninja")

[greptile-apps]

Greptile Overview

Greptile Summary

This PR changes BuildConfig fields from having explicit default values (False, 20, "Release") to None, allowing cmake to use its own default or cached values when these options aren't explicitly set. This addresses issue #5824.

Major changes:

• Changed many boolean fields from bool = False to bool | None = None
• Changed cpp_standard from int = 20 to int | None = None
• Changed build_type from str = "Release" to str | None = None
• Updated cmake command generation to only add options when the corresponding field is not None
• Added None check in setup.py to prevent comparing None < 20

Issues found:

• The no_ninja logic in python/utils.py:328-330 inadvertently changes the default behavior: Ninja will no longer be used by default when no_ninja is None, breaking the previous behavior where Ninja was the default build generator
• The nvmmh_include_dir check in python/utils.py:326-327 will now pass empty strings to cmake, which was previously avoided

Confidence Score: 2/5

• This PR has critical logic errors that change default build behavior
• The PR correctly implements the core concept of using None to defer to cmake defaults, but introduces two significant bugs: (1) Ninja is no longer used by default due to incorrect None handling in the conditional logic, and (2) empty strings for nvmmh_include_dir will now be passed to cmake. These issues will break existing build workflows that rely on Ninja being the default generator.
• python/utils.py requires fixes to the no_ninja and nvmmh_include_dir conditional logic at lines 326-330

Important Files Changed

Filename	Overview
python/utils.py	Changed BuildConfig fields to use None as default; cmake command generation logic has issues with no_ninja and nvmmh_include_dir that alter default behavior
python/setup.py	Added None check for cpp_standard validation; correctly prevents comparison with None

Sequence Diagram

sequenceDiagram
    participant User
    participant setup.py
    participant BuildConfig
    participant utils
    participant CMake

    User->>setup.py: Run setup.py (pip install)
    setup.py->>BuildConfig: Create BuildConfig instance
    Note over BuildConfig: Fields default to None<br/>instead of False/20/"Release"
    setup.py->>utils: override_build_config_from_env(config)
    utils->>BuildConfig: Set fields from env vars (if present)
    Note over BuildConfig: Fields remain None if env var not set
    setup.py->>BuildConfig: Check cpp_standard is not None and >= 20
    setup.py->>utils: run(config, version_tag, relative_path)
    utils->>utils: cmake(config, relative_path)
    utils->>utils: Build cmake command
    loop For each BuildConfig field
        alt Field is not None
            utils->>utils: Add cmake option to cmd_str
        else Field is None
            Note over utils: Skip option, use cmake default/cached value
        end
    end
    utils->>CMake: Execute cmake command
    Note over CMake: Uses default or cached values<br/>for omitted options
    CMake-->>utils: Configuration complete
    utils->>CMake: Build and install
    CMake-->>utils: Build complete
    utils-->>setup.py: Build complete
    setup.py-->>User: Installation complete

Loading

[greptile-apps]

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

This changes the default build behavior. Previously, when no_ninja was False (the default), Ninja was used. Now, when no_ninja is None (the new default), this condition evaluates to False, so Ninja will NOT be used by default.

To maintain the previous behavior, this should be:

Suggested change

	if config.no_ninja is not None and not config.no_ninja:
	cmd_str.append("-G")
	cmd_str.append("Ninja")
	if config.no_ninja is None or not config.no_ninja:
	cmd_str.append("-G")
	cmd_str.append("Ninja")

[greptile-apps]

When nvmmh_include_dir is an empty string (the old default in line 31), this will now pass -DNVMMH_INCLUDE_DIR= to cmake. The previous check if config.nvmmh_include_dir: correctly avoided this. Consider using:

Suggested change

	if config.nvmmh_include_dir is not None:
	cmd_str.append(f"-DNVMMH_INCLUDE_DIR={config.nvmmh_include_dir}")
	if config.nvmmh_include_dir is not None and config.nvmmh_include_dir:
	cmd_str.append(f"-DNVMMH_INCLUDE_DIR={config.nvmmh_include_dir}")