Option to disable skeleton formatting with rustfmt by flxzt · Pull Request #1465 · libbpf/libbpf-rs

flxzt · 2026-10-08T16:41:54Z

Added ability to skip running rustfmt on the skeleton Rust source entirely
in libbpf-cargo.

In the crate, SkeletonBuilder::rustfmt() changed signature to take parameter
Option<PathBuf>.
The CLI gained flag --disable-rustfmt.

d-e-s-o · 2026-10-09T17:23:18Z

+- Added ability to skip running `rustfmt` on the skeleton Rust source entirely in `libbpf-cargo`.
+  In the crate, `SkeletonBuilder::rustfmt()` changed signature to take parameter `Option<PathBuf>`.
+  The CLI gained flag `--disable-rustfmt`.


Please stick to existing format; no sentences.

should be fixed.

d-e-s-o · 2026-10-09T17:27:16Z

+                let rustfmt_path = if disable_rustfmt {
+                    None
+                } else {
+                    Some(rustfmt_path.unwrap_or("rustfmt".into()))


Clearly we don't want to have three copies of the string "rustfmt" floating around everywhere. Introduce a constant?

Sure, will do.

d-e-s-o · 2026-10-09T17:32:40Z

        Command::new(r)
    } else {
-        Command::new("rustfmt")
+        return Ok(Cow::Borrowed(s.as_bytes()));


This function is directly reachable from the public generate and you are changing semantics of said function subtly, for the worse, and for no good reason, from what I gather.

Would skipping try_rustfmt() at caller side be better in your view when rustfmt_path is None (two occasions)?

If you question the change of semantics of the rustfmt argument itself: this was intentional, the fallback is resolved now as early as possible (in main and in SkeletonBuilder) which I thought is cleaner.

Otherwise one would have to deal with something like Option<Option<PathBuf>> where the outer option is resolved to the fallback "rustfmt" executable and the inner one let's it skip or something like that.
That was a pattern I was not comfortable with.

Nested Option is a way, but I tend to agree it's not ideal. My main gripe is that there is effectively no breaking change and yet semantics change, so users won't notice.

Personally, given how niche this entire rustfmt infrastructure is, I'd say we just special case Some("") to map to "skip". I'd think then we can also remove the public "private" constant.

Ok, I have changed it to do that.

d-e-s-o · 2026-10-09T17:34:21Z

    /// Default searches `$PATH` for `rustfmt`
-    pub fn rustfmt<P: AsRef<Path>>(&mut self, rustfmt: P) -> &mut Self {
-        self.rustfmt = rustfmt.as_ref().to_path_buf();
+    pub fn rustfmt(&mut self, rustfmt: Option<PathBuf>) -> &mut Self {


Why are we dropping the P generic?

This is a change to avoid that users have to supply None::<PathBuf>.
Since it is a breaking change anyway, I thought this makes the public API more ergonomic.

It penalizes usage of .rustfmt("xxxx"), though, which now requires an explicit PathBuf conversion first.

I'd think something like .rustfmt::<PathBuf>(None) is quite acceptable.

Ok, with the latest change this is untouched.

d-e-s-o · 2026-10-09T17:36:07Z

+        /// Whether to disable applying rustfmt to the generated skeleton Rust source.
+        #[arg(long)]
+        disable_rustfmt: bool,


IMO this is bad in many ways. For one, at the very least it should be mutually exclusive with rustfmt_path. But also, it seems icky to have a separate argument like that to begin with. Can we fold that into rustfmt_path?

I wondered how to approach this in a better way as well - the reason I opted for this implementation is to avoid breakage as much as possible.
Users that keep invoking the CLI in the same way still fall back to rustfmt, while users that explicitly want to opt out of formatting can do so with the new flag.
I will apply that both arguments are mutually exclusive, for the rest I did not find a better pattern - I'm open to suggestions.

When both options are supplied, the CLI now reports the following error:

$ cargo run -p libbpf-cargo -- libbpf gen --rustfmt-path some_path --disable-rustfmt error: the argument '--rustfmt-path <RUSTFMT_PATH>' cannot be used with '--disable-rustfmt'

Can we map --rustfmt-path="" to None?

Or plumb it through as per earlier comment and then special case where needed.

I have changed it accordingly.

Added ability to skip running `rustfmt` on the skeleton Rust source entirely in `libbpf-cargo` by setting `rustfmt-path` to an empty string for the CLI or passing an empty path to `SkeletonBuilder::rustfmt()`.

d-e-s-o

Seems fine to me, thanks.

Improve the CHANGELOG message for #1465 and the style of some code comments. Signed-off-by: Daniel Müller <deso@posteo.net>

flxzt force-pushed the fz/no-rustfmt branch 4 times, most recently from a47e37b to a11509b Compare October 9, 2026 15:54

d-e-s-o requested changes Oct 9, 2026

View reviewed changes

flxzt force-pushed the fz/no-rustfmt branch 2 times, most recently from 802ee6b to ef77273 Compare October 9, 2026 18:12

flxzt requested a review from d-e-s-o October 9, 2026 18:12

flxzt force-pushed the fz/no-rustfmt branch from ef77273 to dc072fd Compare October 9, 2026 18:21

Option to disable skeleton formatting with rustfmt

dac13c0

Added ability to skip running `rustfmt` on the skeleton Rust source entirely in `libbpf-cargo` by setting `rustfmt-path` to an empty string for the CLI or passing an empty path to `SkeletonBuilder::rustfmt()`.

flxzt force-pushed the fz/no-rustfmt branch from dc072fd to dac13c0 Compare October 11, 2026 14:38

d-e-s-o approved these changes Oct 12, 2026

View reviewed changes

d-e-s-o merged commit ff2b750 into libbpf:master Oct 12, 2026
17 checks passed

d-e-s-o mentioned this pull request Oct 12, 2026

Improve CHANGELOG message for #1465 #1469

Merged

d-e-s-o added a commit that referenced this pull request Oct 12, 2026

Improve CHANGELOG message for #1465

f23d0eb

Improve the CHANGELOG message for #1465 and the style of some code comments. Signed-off-by: Daniel Müller <deso@posteo.net>

Conversation

flxzt commented Oct 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-e-s-o Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-e-s-o left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

d-e-s-o Oct 9, 2026 •

edited

Loading