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

Warn about dragons in the Bazel include path #927

Merged
merged 2 commits into from
Jun 5, 2023

Conversation

drigz
Copy link
Member

@drigz drigz commented May 31, 2023

#837 has caused a couple of disappointments for PR authors, so I'm hoping this comment can save them some time, or even help them towards finding a complete solution for the problem.

#837 has caused a couple of
disappointments for PR authors, so I'm hoping this comment can save them
some time, or even help them towards finding a complete solution for the
problem.
@drigz drigz requested a review from sergiud May 31, 2023 08:10
@drigz
Copy link
Member Author

drigz commented May 31, 2023

@cpsauer Let me know if you think this wording would have been helpful for you / if it could be improved.

@cpsauer
Copy link

cpsauer commented Jun 1, 2023

Hey, Diego! Love that you followed up on it.

It'd have flagged to me that something was up--but probably wouldn't have deduplicated spelunking into the what about the implementation of strip_include_prefix provided that (undocumented) Bazel behavior.

You sure it wouldn't make sense to just quickly slay the dragon and be done with it?
[Maybe the easiest would be to just add an include directory around the glog one, pointing includes at that? Or to do so and then move that to the top level if we wanted to be a bit more standard?]
[You're obviously way more experienced with it than me, but it seems like it's been causing some issues and relies on undocumented bazel behavior that could break things under you someday.]

If not, quick suggest on the comment: How about "[leak private headers like stacktrace.h], because strip_include_prefix's implementation only creates symlinks for the public hdrs[.]"

Thanks for all you guys do!
Chris

@drigz
Copy link
Member Author

drigz commented Jun 5, 2023

You sure it wouldn't make sense to just quickly slay the dragon and be done with it?

I wish this were higher up my list of "most expensive technical debt due to my lack of foresight" :( Thanks for the suggestion on the comment.

@drigz drigz merged commit 3a0d4d2 into master Jun 5, 2023
@drigz drigz deleted the rodrigoq/there-be-dragons branch June 5, 2023 09:08
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.

3 participants