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

build.rs: Use pkg-config to find header search paths for bindgen. #171

Merged

Conversation

francis-starlab
Copy link

This optionally enables using pkg-config to find header search paths and linker paths. The pkg-config crate prints "cargo:rustc-link-lib=blkid" along with the appropriate library search paths.

Additionally this adds an option static feature for statically linking libblkid.

@mulkieran mulkieran requested a review from jbaublitz November 25, 2024 21:17
@mulkieran
Copy link
Member

@jbaublitz Could you take a look at this?

@jbaublitz
Copy link
Member

@francis-starlab Hi, thanks for the contribution! Could you explain a little bit more what your use case is here or the problem that you're running into that this resolves?

From what I can tell, one thing you want is static compilation. Is that correct?

For the include flags, can you explain a bit more what the problem is you're bumping into with the lack of using pkg-config to provide the include flags?

I basically want to have a better understanding of whether these changes should be the default and will benefit all of our users.

@francis-starlab
Copy link
Author

Could you explain a little bit more what your use case is here or the problem that you're running into that this resolves?

We are cross compiling internal rust (and c and c++) in an environment with non-standard header locations. It's generally more convenient/consistent with other code for us to use pkg-config to provide header/linker paths. Otherwise we can use build.rs and various bindgen env vars, so this is mostly just to simplify things. We also link thinks statically, so that's also for helping simplify things for us.

@jbaublitz
Copy link
Member

Could you explain a little bit more what your use case is here or the problem that you're running into that this resolves?

We are cross compiling internal rust (and c and c++) in an environment with non-standard header locations. It's generally more convenient/consistent with other code for us to use pkg-config to provide header/linker paths. Otherwise we can use build.rs and various bindgen env vars, so this is mostly just to simplify things. We also link thinks statically, so that's also for helping simplify things for us.

Are you currently using the env var for bindgen to enable these currently? If so, I think it's worth taking this PR and generalizing it to all of our bindings to ease this use case for all of them.

@francis-starlab
Copy link
Author

Are you currently using the env var for bindgen to enable these currently?

Currently we are using a forked libblkid-rs, though prior to that we where using BINDGEN_EXTRA_CLANG_ARGS to pass header paths.

If so, I think it's worth taking this PR and generalizing it to all of our bindings to ease this use case for all of them.

I assume we are talking about the following?

https://github.com/stratis-storage/libcryptsetup-rs/tree/master/libcryptsetup-rs-sys
https://github.com/stratis-storage/devicemapper-rs

I can most likely make similary such PRs against those if desired.

@jbaublitz
Copy link
Member

Yes, that would be great, thanks! I'm going to try to get to review today of this PR and then we can move on to the other bindings.

Copy link
Member

@jbaublitz jbaublitz left a comment

Choose a reason for hiding this comment

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

I'll merge it after you make this change and then if you're willing to put up something similar for the other bindings, that would be wonderful. Thanks for your contribution!

libblkid-rs-sys/build.rs Outdated Show resolved Hide resolved
@francis-starlab
Copy link
Author

I have made a similar/equivalent PR for devicemapper-rs here: stratis-storage/devicemapper-rs#952

https://github.com/stratis-storage/libcryptsetup-rs already uses pkg-config and therefore needs no such MR.

@jbaublitz
Copy link
Member

@francis-starlab Could you make one more PR for libcryptsetup-rs?

Copy link
Member

@jbaublitz jbaublitz left a comment

Choose a reason for hiding this comment

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

I'm happy to do the rebasing and merging if you're okay with it @francis-starlab. Any objection to merging this @mulkieran?

@mulkieran
Copy link
Member

@francis-starlab Can you rebase, please?

@jbaublitz
Copy link
Member

jbaublitz commented Dec 18, 2024

@francis-starlab It appears I can't rebase this PR because of some setting not letting me update your repository like I have been able to with other external contributors. If you rebase, I'll immediately merge.

@francis-starlab
Copy link
Author

@francis-starlab It appears I can't rebase this PR because of some setting not letting me update your repository like I have been able to with other external contributors. If you rebase, I'll immediately merge.

rebased.

@jbaublitz jbaublitz merged commit 35ae0e9 into stratis-storage:master Dec 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done(3)
Development

Successfully merging this pull request may close these issues.

3 participants