-
Notifications
You must be signed in to change notification settings - Fork 748
Add Builder functions for including folder(s) and headers #2123
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
@emilio for your review or referral to a reviewer |
@aatifsyed it has been a while so the logs are gone. Do you happen to know why CI was failing? Regarding the PR itself. I'd use |
1846a9d
to
7867787
Compare
@pvdrz Thanks for resurrecting this!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can change
includes
by something less similar toinclude
likeinclude_folder
or something? I'm bad at naming things so deferring to @emilio or @kulp for a review.
I am probably not better than @pvdrz at naming things. I agree that include
and includes
are too similar; but the new function headers
is too close to the existing function header
, too.
The only thing that comes to mind is include_multiple
and header_multiple
but I do not really like those either.
I am not standing in the way of this but for the moment I am a little wary of cluttering the API.
src/lib.rs
Outdated
} | ||
|
||
/// Include multiple folders for system header file resolution | ||
pub fn includes<T: AsRef<str>>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think having both includes
and include
is probably not necessary, except for consistency with headers
and header
; if we were starting from scratch, I would prefer just to have the plural versions, and call them with an array of size one, or std::iter::once
, if I have only one element. But maybe that is just me.
But removing header
would break backward compatibility, and having both header
and headers
but only includes
seems inconsistent, so maybe I do not have anything better to suggest here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include_directories
?
maybe we can use |
☔ The latest upstream changes (presumably #2284) made this pull request unmergeable. Please resolve the merge conflicts. |
Hello all, thanks for your feedback :) I'm away from my PC for a couple of weeks, so I'll catch up and amend when I'm back. I'd be interesting in feedback on the following suggestion:
So the documented API remains smaller? We could even |
I'd favor |
☔ The latest upstream changes (presumably 2034a0f) made this pull request unmergeable. Please resolve the merge conflicts. |
See #2743. |
:/ |
Possibly I was too picky about the naming of things. The bindgen project has varying amounts of volunteer support; sometimes one person gets lucky, and another unlucky. Still, I am sorry, @aatifsyed, that your PR did not get merged. |
I find myself frequently extending
bindgen::Builder
with the following functions:This pull request adds them.
Points to consider
-I
vs--include-directory=
pkg-config
, a motivating usecase forincludes