Skip to content

Conversation

chenfengjin
Copy link
Contributor

@chenfengjin chenfengjin commented Sep 14, 2025

Purpose

vLLM use some c++14/c++ 17 features, which may result in compilation error.

  • std::decay_t used in scaler_types.hpp ,required c++14 or above
  • std::optional in many files, which required c++ 17 or above

Force use 17 as default CXX version may reduce many confusing errors when build from source on some platforms that use C++ 11 as default CXX standard like MacOS apple silicon

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors.

You ask your reviewers to trigger select CI tests on top of fastcheck CI.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

🚀

@mergify mergify bot added the ci/build label Sep 14, 2025
Copy link

pytorch-bot bot commented Sep 14, 2025

No ciflow labels are configured for this repo.
For information on how to enable CIFlow bot see this wiki

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly enforces the C++17 standard across the project by setting CMAKE_CXX_STANDARD and CMAKE_CXX_STANDARD_REQUIRED in the root CMakeLists.txt. This is a good practice for ensuring build consistency. However, this change introduces a conflict with an existing per-target C++ standard setting, which could lead to maintenance issues in the future. I've added a comment with details on how to resolve this.

Copy link

pytorch-bot bot commented Sep 14, 2025

No ciflow labels are configured for this repo.
For information on how to enable CIFlow bot see this wiki

@pytorch-bot pytorch-bot bot removed the ci/build label Sep 14, 2025
@mergify mergify bot added the ci/build label Sep 14, 2025
@chenfengjin chenfengjin changed the title Force use C++17 to avoid compilation error Force use C++17 globally to avoid compilation error Sep 14, 2025
Copy link

pytorch-bot bot commented Sep 14, 2025

No ciflow labels are configured for this repo.
For information on how to enable CIFlow bot see this wiki

@pytorch-bot pytorch-bot bot removed the ci/build label Sep 14, 2025
@mergify mergify bot added the ci/build label Sep 14, 2025
Copy link

pytorch-bot bot commented Sep 14, 2025

No ciflow labels are configured for this repo.
For information on how to enable CIFlow bot see this wiki

@cjackal
Copy link
Contributor

cjackal commented Sep 14, 2025

Interestingly mergify bot and pytorch bot are battling here 🤣

Signed-off-by: chenfengjin <1871653365@qq.com>
Copy link

pytorch-bot bot commented Sep 14, 2025

No ciflow labels are configured for this repo.
For information on how to enable CIFlow bot see this wiki

Copy link
Collaborator

@LucasWilkinson LucasWilkinson left a comment

Choose a reason for hiding this comment

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

LGTM; thanks for the contribution!

@LucasWilkinson LucasWilkinson enabled auto-merge (squash) September 14, 2025 17:40
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 14, 2025
@LucasWilkinson LucasWilkinson merged commit 79cbcab into vllm-project:main Sep 14, 2025
92 of 93 checks passed
dsxsteven pushed a commit to dsxsteven/vllm_splitPR that referenced this pull request Sep 15, 2025
bbartels pushed a commit to bbartels/vllm that referenced this pull request Sep 15, 2025
Signed-off-by: chenfengjin <1871653365@qq.com>
Signed-off-by: bbartels <benjamin@bartels.dev>
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants