Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow custom values of CMAKE_BUILD_TYPE #17106

Open
rojkov opened this issue Jun 23, 2021 · 5 comments
Open

Allow custom values of CMAKE_BUILD_TYPE #17106

rojkov opened this issue Jun 23, 2021 · 5 comments
Labels
area/build enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@rojkov
Copy link
Member

rojkov commented Jun 23, 2021

Title: Allow custom values of CMAKE_BUILD_TYPE

Description:
Currently Envoy's build system sets CMAKE_BUILD_TYPE to Bazel unconditionally (since #8280). Yet some CMake-based dependencies modify e.g. defines or CFLAGS for different build types. The conventional values are Debug, Release, RelWithDebInfo etc.

It would be nice for debug purposes (or for enabling optimizations) to override CMAKE_BUILD_TYPE in the envoy_cmake_external() rule.
@rojkov rojkov added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Jun 23, 2021
@alyssawilk alyssawilk added area/build and removed triage Issue requires triage labels Jun 23, 2021
@alyssawilk
Copy link
Contributor

cc @lizan @phlax

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 23, 2021
@github-actions
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@keith
Copy link
Member

keith commented Aug 11, 2021

Maybe we should mark this one as no stalebot? I discovered another issue with this updating rules_foreign_cc, it seems that some nested configs like IMPORTED_LOCATION_DEBUG are being applied in dependencies, and do not cover our Bazel config, therefore leading to build failures. I'm kinda surprised this isn't something causing more issues today. I tracked down the addition of this override to d3b7ffd#diff-8a0e11e60080180107a036de4ee071e6d8855f64b61b5862874b7c5746716ddcR72 which doesn't really explain why that was needed specifically, but maybe @wrowe and @lizan remember

keith added a commit to keith/envoy that referenced this issue Aug 11, 2021
@keith
bazel: remove CMAKE_BUILD_TYPE override
9ccad15 
rules_foreign_cc sets this automatically to either Debug or Release
based on bazel's compilation_mode. Overriding this can cause issues when
deps' cmake configs expect to be in a specific subset of configs. Unlike
LLVM most cmake configs seem to silently fail in that case by not
setting an expected var correctly. I hit this issue when updating
rules_foreign_cc (I'm not sure of why the update broke it) and wamr
missing some settings for the custom Bazel config.

Fixes: envoyproxy#17106

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith keith mentioned this issue Aug 11, 2021
Closed
@keith
Copy link
Member

keith commented Aug 11, 2021

Testing if removing it works at this point #17679

@rojkov rojkov reopened this Aug 12, 2021
@rojkov rojkov added help wanted Needs help! and removed stale stalebot believes this issue/PR has not been touched recently labels Aug 12, 2021
@rojkov rojkov mentioned this issue Sep 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/build enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bazel: remove CMAKE_BUILD_TYPE override keith/envoy
3 participants
@rojkov @keith @alyssawilk