Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

aatifsyed
Copy link

@aatifsyed aatifsyed commented Dec 4, 2021

I find myself frequently extending bindgen::Builder with the following functions:

    /// Add multiple input C/C++ headers.
    pub fn headers<T: AsRef<str>>(
        mut self,
        headers: impl IntoIterator<Item = T>,
    ) -> Builder { ... }

    /// Include a folder for system header file resolution
    pub fn include(self, path: impl AsRef<str>) -> Builder {
        self.clang_arg(format!("-I{}", path.as_ref()))
    }

    /// Include multiple folders for system header file resolution
    pub fn includes<T: AsRef<str>>(
        mut self,
        paths: impl IntoIterator<Item = T>,
    ) -> Builder { ... }

This pull request adds them.

Points to consider

  • use of -I vs --include-directory=
  • Could add documentation to refer to pkg-config, a motivating usecase for includes

@aatifsyed
Copy link
Author

@emilio for your review or referral to a reviewer

@pvdrz
Copy link
Contributor

pvdrz commented Sep 22, 2022

@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 impl AsRef<str> instead of impl AsRef<Path> as there's no consensus about how to handle invalid utf-8 paths yet.

@aatifsyed aatifsyed force-pushed the master branch 2 times, most recently from 1846a9d to 7867787 Compare September 22, 2022 22:28
@aatifsyed
Copy link
Author

aatifsyed commented Sep 22, 2022

@pvdrz Thanks for resurrecting this!

  • I've made your suggested change from Path to str
  • I've rebased to the latest master, with gratitude to this guide
  • I'm afraid I don't remember why the pipelines were failing

@pvdrz
Copy link
Contributor

pvdrz commented Sep 23, 2022

Looks good to me!

Maybe we can change includes by something less similar to include like include_folder or something? I'm bad at naming things so deferring to @emilio or @kulp for a review.

@pvdrz pvdrz requested review from emilio and kulp and removed request for emilio September 26, 2022 16:31
Copy link
Member

@kulp kulp left a 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 to include like include_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>>(
Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include_directories?

@pvdrz
Copy link
Contributor

pvdrz commented Oct 3, 2022

Maybe we can change includes by something less similar to include like include_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.

maybe we can use include_directory? It'd be clear enough I think

@bors-servo
Copy link

☔ The latest upstream changes (presumably #2284) made this pull request unmergeable. Please resolve the merge conflicts.

@aatifsyed
Copy link
Author

aatifsyed commented Oct 5, 2022

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:

  • #[doc(hidden)] the existing header function. Reduces API clutter without deprecating
  • add headers(self, i: impl IntoIterator<Item = impl Into<String>>)
  • add include_directories(self, i: impl IntoIterator<Item = impl Into<String>>)

So the documented API remains smaller?

We could even #[deprecated] header, but I'm wary of that

@pvdrz
Copy link
Contributor

pvdrz commented Oct 5, 2022

I'd favor include_directories(self, i: impl IntoIterator<Item = impl Into<String>>). We would have to document clearly that people are supposed to do include_directories([dir]) for a single directory.

@bors-servo
Copy link

☔ The latest upstream changes (presumably 2034a0f) made this pull request unmergeable. Please resolve the merge conflicts.

@aatifsyed aatifsyed closed this Jul 19, 2023
@kulp
Copy link
Member

kulp commented Feb 9, 2024

See #2743.

@aatifsyed
Copy link
Author

:/

@kulp
Copy link
Member

kulp commented Feb 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants