Use bindgen as a program rather than as a library

[ojeda]

This allows us to remove the kernel build.rs script
and simplifies the build. Cargo.lock is tiny now.
The shlex dependency also goes away.

Note that this commit does not use the list of included
functions/types/vars etc., so it is not equivalent.

Signed-off-by: Finn Behrens [email protected]
Signed-off-by: Miguel Ojeda [email protected]

[ojeda]

This is Finn's from #23 split into only the bindgen part and a few extra cleanups.

We probably want to discuss a bit this one. @alex @geofft Any comments? I am not sure why you used the included types/functions/vars functionality in the build.rs script -- perhaps to minimize size or to avoid some warnings?

Finn also mentions in #32:

What's the direction we want to go with build.rs? I'm more a fan of removing it, as we only have to have bingen, and not some libclang path set.

It definitely looks simpler and it is also quicker to build. So unless there is a reason not to do it this way, I would go for it.

[alex]

The reason we had the explicit list was because there were some types/functions that could not be generated safely (e.g. due to large arrays that Rust didn't impl some traits for) -- perhaps all those issues are fixed now.

[kloenk]

Bindgen worked and compiled for me, just had the problem with builtin. That did not have to come from bindgen

[ojeda]

It worked for me too -- there is a single warning, though:

warning: `#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)
     --> include/generated/rust_bindings.rs:17212:10
      |
17212 | #[derive(Debug, Default)]
      |          ^^^^^
      |
      = note: `#[warn(safe_packed_borrows)]` on by default
      = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
      = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>
      = note: this warning originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

Perhaps we can exclude rather than include?

        --blacklist-function <function>...         Mark <function> as hidden.
        --blacklist-item <item>...                 Mark <item> as hidden.
        --blacklist-type <type>...                 Mark <type> as hidden.

[ojeda]

The CI needs a small change to avoid this warning:

error: 'rustfmt' is not installed for the toolchain 'nightly-2020-08-27-x86_64-unknown-linux-gnu'
To install, run `rustup component add rustfmt`
Failed to run rustfmt: Internal rustfmt error (non-fatal, continuing)

In the docs, as well, mentioning bindgen is required.

[kloenk]

I think I disabled rustfmt for the bindings in my pr. Not sure which flag it was though

[ojeda]

I think I disabled rustfmt for the bindings in my pr. Not sure which flag it was though

--no-rustfmt-bindings; I removed it to match better the original settings in build.rs.

I think it is worth to have rustfmt. We could avoid the dependency if it was a third-party tool, but since it comes with the compiler (in the standalone installers), I think it is fair to require it.

I will update the CI and run it again.

[ojeda]

Nowadays the default profile for rustup includes rustfmt, though, so I am not sure why the CI complains. Perhaps the image uses the minimal one...

[kloenk]

I'm strongly against rustup. I don't see a reason to fmt it, as it's not code for user api. So I would not fmt it. So it does not break in a context without rustup, rustfmt.

[kloenk]

I think it is worth to have rustfmt. We could avoid the dependency if it was a third-party tool, but since it comes with the compiler (in the standalone installers), I think it is fair to require it.

It does not come if you don't use rustup. I see the usecase of nix, where I have to figure out how to create rust-src without rustup, and that I will only put rustc and cargo into the sandbox, so there would not be a rustfmt in some build envs.

[ojeda]

I'm strongly against rustup. I don't see a reason to fmt it, as it's not code for user api. So I would not fmt it. So it does not break in a context without rustup, rustfmt.

(Did you mean against rustfmt?) Note that we don't require rustup -- it is just in the CI here that we use it. In the quick start guide I tell people that they can use either the standalone installers or rustup (or, of course, their own distro packages -- we should mention that too).

As for formatting the bindings or not, I think it is useful to have the file formatted, specially since people will need to take a look from time to time. On its own, rustfmt may be useful later too if we decide to tell people to use it (I am trying to get more people to use clang-format for C in the kernel, and it is not easy... :-)

In any case, rustfmt not being found by bindgen does not fail the build even if it is not there (as you see in the CI above).

[ojeda]

I see the usecase of nix, where I have to figure out how to create rust-src without rustup, and that I will only put rustc and cargo into the sandbox, so there would not be a rustfmt in some build envs.

Hmm... do you think it is that hard to get in in those environments? I mean, if one can build Rust, one can enable it during build, so most distros/package managers should offer it at some point, no?

[ojeda]

Force-pushed changes:

• Mentioned bindgen in the quick start guide.
• Fixed the rustfmt bit in the CI.
• Removed last mention of shlex removed in Cargo.lock. For some reason, Cargo was not removing it, but if you let it re-create the file from scratch, it goes away as expected.

The only remaining thing is the warning. I think it is OK to leave it like that for the moment -- hopefully it will resolve itself since other people seem to have hit the same issue with bindgen. If not, we can exclude the offending entity later on.

So this is ready to go.

[kloenk]

I see the usecase of nix, where I have to figure out how to create rust-src without rustup, and that I will only put rustc and cargo into the sandbox, so there would not be a rustfmt in some build envs.

Hmm... do you think it is that hard to get in in those environments? I mean, if one can build Rust, one can enable it during build, so most distros/package managers should offer it at some point, no?

You can enable it in the env. But it's one dependency more you have to download. And it makes the closure bigger. That's why I would like to keep rustfmt optional.

[kloenk]

LGTM, only the rustfmt thing :-)

[ojeda]

You can enable it in the env. But it's one dependency more you have to download. And it makes the closure bigger. That's why I would like to keep rustfmt optional.

I see, makes sense. To avoid the benign error message, we could detect if it is installed or not, and use the flag accordingly. Do you want to give it a go?

[kloenk]

I see, makes sense. To avoid the benign error message, we could detect if it is installed or not, and use the flag accordingly. Do you want to give it a go?

Sounds good. If you can implement that, great.

[ojeda]

I just realized that, for some reason, bindgen does not need to be installed in the CI (?). Howevers, the docs should explain how to install it. Let me do that...

[kloenk]

I just realized that, for some reason, bindgen does not need to be installed in the CI (?). Howevers, the docs should explain how to install it. Let me do that...

Maybe it was still in the cache? I think I installed it with cargo install bindgen, but not sure. I only know that I cannot use the bindgen from nixpkgs, as it still uses clang 9 xD

[ojeda]

Maybe it was still in the cache? I think I installed it with cargo install bindgen, but not sure. I only know that I cannot use the bindgen from nixpkgs, as it still uses clang 9 xD

The CI should be spinning up a new system every time from the image, otherwise it would be non-deterministic, so I don't know. Anyway, the CI is not important as long as it works :-)

[ojeda]

OK, I rewrote a bit the quick start -- now the different tools are split into several sections.