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

Switch strip_include_prefix -> includes #926

Closed
wants to merge 2 commits into from
Closed

Switch strip_include_prefix -> includes #926

wants to merge 2 commits into from

Conversation

cpsauer
Copy link

@cpsauer cpsauer commented May 28, 2023

Hey Googlers!

I was helping a Googler use a bazel tool I'd written over in hedronvision/bazel-compile-commands-extractor#121 and noticed that the BUILD file used a strip_include_prefix where an includes will do. Swiching to includes would save some symlinking (see bazelbuild/bazel#17591) and probably the windows pain you guys were experiencing there.

So I figured toss up a quick PR to change it over, fixing his issue and trying to leave things better than I found them. Second commit coming in a sec to propose unwinding the windows hack, now that strip_include_prefix is no longer in play.

Cheers,
Chris
(ex-Googler and author of some Bazel tools)

@drigz
Copy link
Member

drigz commented May 30, 2023

Hey Chris, thanks for sending this. We should add a comment but strip_include_prefix is there for a reason: #837 (comment) When setting up these build rules we found a bunch of cases where Bazel exposed "private" headers to downstream targets and I'm reluctant to change anything here as my understand has only gotten rustier since then.

As far as I'm aware, this change will mean that downstream targets that use #include <stacktrace.h> might pick up glog's internal header rather than their own headers as they expect. Unfortunately, I'm not aware of a good way to test for this (until thinking about it just now, I'd thought we'd need an external repository, although maybe we could have a test within this repo that does #ifdef #error for a header defined in stacktrace.h...). Can you check whether this PR will change that behavior?

@cpsauer
Copy link
Author

cpsauer commented May 31, 2023

Hey, Rodrigo! Indeed it would. Great, subtle catch. Thanks for defending the functionality, and also for being nice about it. It hadn't struck me that one might use the individual symlinking of _virtual_includes to get bazel to enforce the header inclusion restrictions Bazel otherwise doesn't without --features=layering_check. Agree that it's probably worth a quick comment, since it's depending on an implementation detail, but it is both important and clever! Anyway, closing down this ham-handed attempt at helping out.

Thanks again for your careful eye!
Chris

P.S. After a little more thought, I suppose another approach might be to switch to header names that clearly don't collide with system ones--and therefore resolve some other ambiguities, but I should probably bow out.

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