-
Notifications
You must be signed in to change notification settings - Fork 775
Add Builder functions for including folder(s) and headers #2123
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
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!
|
kulp
left a comment
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
includesby something less similar toincludelikeinclude_folderor 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::Builderwith the following functions:This pull request adds them.
Points to consider
-Ivs--include-directory=pkg-config, a motivating usecase forincludes