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.

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

support selectively disabling NS_BLOCK_ASSERTIONS for opt builds #341

Merged

Conversation

aaronsky
Copy link
Contributor

maintains the default behavior of passing -DNS_BLOCK_ASSERTIONS=1 for opt builds. for builds where this is undesirable, set --features=-ns_block_assertions to unconditionally not-set this flag, without having to unset the entire default_compile_flags feature.

does not add tests, but i can add them if requested.

@keith
Copy link
Member

keith commented Aug 12, 2024

previously this was always enabled, for dbg/fastbuild/opt, now it looks like it's only enabled for opt? i think tests would be nice to cover the behavior here

@aaronsky
Copy link
Contributor Author

aaronsky commented Aug 12, 2024

@keith From what I can tell, that isn't the case. My understanding is that -DNS_BLOCK_ASSERTIONS=1 is only being set in opt builds. I could easily be misinterpreting the existing code, but I was able to confirm that understanding in my own builds.

I am willing to write tests, but was having trouble finding any existing clang flag tests. Reusing the bits for linker tests wasn't working for me. Do you know where I could find them?

@keith
Copy link
Member

keith commented Aug 12, 2024

oh you're right, the constraint was folded below more lines than i expected. here's a file you can add tests in #342

@aaronsky aaronsky force-pushed the aaronsky/ns_block_assertions_feature branch from 588a3b6 to 88153a1 Compare August 12, 2024 17:00
@keith keith merged commit 691a7b1 into bazelbuild:master Aug 12, 2024
11 checks passed
@keith
Copy link
Member

keith commented Aug 12, 2024

thanks!

@aaronsky aaronsky deleted the aaronsky/ns_block_assertions_feature branch August 12, 2024 17:14
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.

4 participants