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

modules/rust: allow customizing env vars on bindgen #13631

Closed

Conversation

eternalNight
Copy link
Contributor

Bindgen is configurable through environment variables on how it searches for clang and what extra arguments it passes to the compiler. The former is the only way to change which clang or libclang bindgen will use, which is useful to work around the issue that bindgen uses clang and libclang of different versions on systems with multiple version of the compiler installed.

Add an env keyword argument to bindgen() of the rust module to allow customizing the environment variables when invoking bindgen. The argument has the same type as env of custom_target().

@dcbaker
Copy link
Member

dcbaker commented Sep 4, 2024

You can already pass additional arguments to clang using the c_args argument, which uses the documented bindgen <args> -- <clang_args> format, avoiding the need to wrap the bindgen call in an extra layer.

rust.bindgen(input : 'in.h', output : 'out.rs', c_args : '-DEXPOSE_API')

Looking at the build scripts for sys-clang, it appears only the clang binary can be changed at runtime using CLANG_PATH, and the other options are set at compile time and not at runtime. messing with CLANG_PATH directly in the build script seems fraught with creating scripts that only work in one build environment, and are not portable, further you need to be sure that your clang matches the libclang that bindgen has been built against.

If the concern is a clang/libclang mismatch, we should solve that, but in general this seems like the primary use of this would be to do something that can already be done, without any benefit over the current mechanism.

Bindgen is configurable through environment variables on how it searches
for clang and what extra arguments it passes to the compiler. The former
is the only way to change which clang or libclang bindgen will use,
which is useful to work around the issue that bindgen uses clang and
libclang of different versions on systems with multiple version of the
compiler installed.

Add an env keyword argument to bindgen() of the rust module to allow
customizing the environment variables when invoking bindgen. The
argument has the same type as env of custom_target().

Signed-off-by: Junjie Mao <junjie.mao@hotmail.com>
@eternalNight
Copy link
Contributor Author

You can already pass additional arguments to clang using the c_args argument, which uses the documented bindgen <args> -- <clang_args> format, avoiding the need to wrap the bindgen call in an extra layer.

rust.bindgen(input : 'in.h', output : 'out.rs', c_args : '-DEXPOSE_API')

Yes, the current c_args argument already allows customizing the command line parameters to be passed to libclang. I use that in the test cases because the other variables, namely CLANG_PATH and LIBCLANG_PATH, are hard to exercise without deep knowledge on where clang/libclang are installed and how multiple versions of them coexist.

Looking at the build scripts for sys-clang, it appears only the clang binary can be changed at runtime using CLANG_PATH, and the other options are set at compile time and not at runtime.

In addition to those that appears in build.rs, clang-sys also reads LIBCLANG_PATH when determining the libclang.so to be dynamically loaded (https://github.com/KyleMayes/clang-sys/blob/master/build/dynamic.rs#L159), and bindgen by default configures clang-sys to load libclang.so dynamically.

messing with CLANG_PATH directly in the build script seems fraught with creating scripts that only work in one build environment, and are not portable, further you need to be sure that your clang matches the libclang that bindgen has been built against.

I agree that tweaking CLANG_PATH while keeping clang-sys to search for libclang.so is problematic. My intention is to set both CLANG_PATH and LIBCLANG_PATH to binaries of the same version to avoid issues like rust-lang/rust-bindgen#2682.

If the concern is a clang/libclang mismatch, we should solve that, but in general this seems like the primary use of this would be to do something that can already be done, without any benefit over the current mechanism.

+1 for solving the mismatch. The author of clang-sys had tried a fix which turned out (unfortunately) to break someone else's build. In the end the author commented that:

I don't think there's any winning here, any change to the status quo w.r.t. Clang executable selection will probably just break someone else's builds. I think folks'll just need to use CLANG_PATH and/or LLVM_CONFIG_PATH in cases like my example above.

The full conversation can be found in rust-lang/rust-bindgen#2682.

@eternalNight
Copy link
Contributor Author

An alternative to avoid rust-lang/rust-bindgen#2682 can be that bindgen() accepts a clang argument of type external_program. If that is provided, meson searches for the corresponding libclang.so via clang -print-file-name=libclang-<version>.so and automatically sets CLANG_PATH and LIBCLANG_PATH.

@dcbaker
Copy link
Member

dcbaker commented Sep 5, 2024

Sigh, that's a frustrating set of problems to have.

Short of being able to change bindgen/sys-libclang to have an interface for this; my preference would be to either expose a specific field for this, or to just silently handle this internally with some sort of heuristic such that:

  • if the (build machine) C, C++, ObjC, or ObjC++ compiler is clang, then use that clang (or the clang next to clang++) and find a matching libclang
  • otherwise if clang is in the binary table of the corresponding machine file use that
  • finally, if the user has already found libclang (maybe with Add specific dependency handling for clang #13134), then use that and try to find a matching clang binary
  • finally, check the default clang binary, and find a matching libclang

I'd really prefer a solution where the end user doesn't need to do anything special to get correct, reliable results, if at all possible.

@eternalNight
Copy link
Contributor Author

Got your point and thanks for the suggestions.

I'll also check if this can be fixed by always finding clang/libclang of the same version in the sys-clang crate. If that doesn't work, I'll submit something following your recommendations.

@dcbaker
Copy link
Member

dcbaker commented Sep 5, 2024

Thanks! I really appreciate this work. We haven't run into this in Mesa yet, but I'm sure we will given enough time :/

@bonzini
Copy link
Collaborator

bonzini commented Sep 6, 2024

  • if the (build machine) C, C++, ObjC, or ObjC++ compiler is clang, then use that clang (or the clang next to clang++) and find a matching libclang
  • otherwise if clang is in the binary table of the corresponding machine file use that

Agreed with this. Note however that this should be the host machine's compiler. Even better: bindgen could have a native argument and pick the right clang.

I don't think this is possible, unfortunately. While it is easy to go from clang to libclang, there's no good way to do the opposite.

  • finally, check the default clang binary, and find a matching libclang

I think this would be the same as step 2, effectively find_program('clang').

@bonzini
Copy link
Collaborator

bonzini commented Sep 6, 2024

clang -print-file-name=libclang-<version>.so cannot work when cross compiling, it will do the equivalent of a native: false library search. Instead we need to find what is effectively the first of

dependency('libclang', native: true, static: false)
dependency('libclang-' + major_version, native: true, static: false)

and point LIBCLANG_PATH to that. So it does probably have a dependency on #13134.

@eternalNight
Copy link
Contributor Author

Close this PR. It is clear that allowing arbitrary environment variables for bindgen is not a proper direction to go at the moment.

The author of clang-sys crate is considering adding a feature to enforce clang/libclang version synchronization (see rust-lang/rust-bindgen#2682), which can greatly simply the logic needed in meson. I'll monitor how that goes.

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