Skip to content

Rename output of -ebpf crate - #146

Open
miniduikboot wants to merge 3 commits into
aya-rs:mainfrom
miniduikboot:main
Open

Rename output of -ebpf crate#146
miniduikboot wants to merge 3 commits into
aya-rs:mainfrom
miniduikboot:main

Conversation

@miniduikboot

@miniduikboot miniduikboot commented Apr 29, 2025

Copy link
Copy Markdown

This makes it possible to run cargo doc --workspace and get both the eBPF and normal crate and their dependencies in the output docs.

Fixes: aya-rs/aya#1260


This change is Reviewable

@miniduikboot

Copy link
Copy Markdown
Author

I chose {{project-name}}_ebpf instead of {{project-name}}-ebpf because then the build fails:

  Error: failed to copy "/tmp/qwe/target/debug/build/qwe-7f193578cdf8454a/out/qwe-ebpf/bpfel-unknown-none/release/qwe-ebpf" to "/tmp/qwe/target/debug/build/qwe-7f193578cdf8454a/out/qwe-ebpf"

  Caused by:
      Is a directory (os error 21)

Other than that, if you have any other suggestions for a better output name, feel free to change it.

@tamird

tamird commented Apr 29, 2025

Copy link
Copy Markdown
Member

Can you add doc generation to CI so we can ensure it doesn't break?

@ErrorTeaPot

Copy link
Copy Markdown

I have just tried the solution in the PR and I have the {{project_name}}_ebpf in the "crates" section, but the content in this new menu is empty.
The "source" button shows the source code of the lib.rs file.

@tamird

tamird commented Sep 6, 2025

Copy link
Copy Markdown
Member

@miniduikboot can you address @ErrorTeaPot's comment?

@tamird tamird left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

@miniduikboot

Copy link
Copy Markdown
Author

Hi Tamir,

I think I understand the comment: when you generate the cargo docs, it uses the source code of the library, which is not interesting compared to the source code of the bin target. It does however add the dependencies of the {{project_name}}-ebpf crate, which I found interesting enough to PR this in. ErrorTeaPot wanted me to take this second step, and add the bin source code to the docs.

It looks like cargo does this because the binary and library targets have the same name: https://github.com/rust-lang/cargo/blob/3b379fcc541b39321a7758552d37e5e0cc4277b9/src/doc/src/reference/cargo-targets.md#the-doc-field

Setting doc = false on the library target leads to no docs at all, so this is not an option. Removing the library target is also not an option, as this throws the warning warning: {{project_name}} v0.1.0 (/tmp/a/{{project_name}}/{{project_name}}) ignoring invalid dependency {{project_name}}-ebpf which is missing a lib target.

If I rename the library target and also set doc = false, it adds the app source code to rustdoc and skips the lib source. I however don't know what the further consequences are of this wrt the dependency to the ebpf crate in {{project_name}}/Cargo.toml: renaming it to {{project_name}}_ebpf_dummy is not possible as that package doesn't exist. I have no clue however why this works like this.

I see two options:

  • We accept the PR as of 679eaab and accept this limitation
  • Someone convinces themself that renaming the dummy library is fine, and we fix this limitation. I'm not familiar enough with the innards of Cargo to make this call.

@tamird

tamird commented Sep 7, 2025

Copy link
Copy Markdown
Member

I think rename the library makes sense. How come you added | snake_case in just one place? I think you need to rebase to clear the test failures. cc @vadorovsky

This makes it possible to run `cargo doc --workspace` and get both the
eBPF and normal crate and their dependencies in the output docs.

Fixes: aya-rs/aya#1260
If a crate has both a lib and a bin target, cargo will prefer to
generate for the library target[1]. By renaming the library target, we
force cargo to document the binary instead.

[1]: https://github.com/rust-lang/cargo/blob/3b379fcc541b39321a7758552d37e5e0cc4277b9/src/doc/src/reference/cargo-targets.md#the-doc-field
@miniduikboot

Copy link
Copy Markdown
Author

snake_case is necessary to prevent this compile error:

error: failed to load manifest for workspace member `/private/var/folders/y6/nj790rtn62lfktb1sh__79hc0000gn/T/tmp.OmrWeNbqrI/aya-test-crate/aya-test-crate`
referenced by workspace at `/private/var/folders/y6/nj790rtn62lfktb1sh__79hc0000gn/T/tmp.OmrWeNbqrI/aya-test-crate/Cargo.toml`

Caused by:
  failed to load manifest for dependency `aya-test-crate-ebpf`

Caused by:
  failed to parse manifest at `/private/var/folders/y6/nj790rtn62lfktb1sh__79hc0000gn/T/tmp.OmrWeNbqrI/aya-test-crate/aya-test-crate-ebpf/Cargo.toml`

Caused by:
  library target names cannot contain hyphens: aya-test-crate_ebpf_dummy

I'll rebase to see if the issue goes away

@tamird

tamird commented Sep 7, 2025

Copy link
Copy Markdown
Member

Ah it seems a real issue caused by this change. The panic handler is now defined twice (lib and bin)

@miniduikboot

Copy link
Copy Markdown
Author

Yeah, but I'm not sure why it's triggered now and only on this example, as I didn't change anything wrt the definition of the panic handler

@tamird

tamird commented Sep 7, 2025

Copy link
Copy Markdown
Member

Ah, it's because of unification. See aya-rs/aya@3078e5a.

sk_msg is the only variant that uses the common crate.

{%- when "sk_msg" %}
use aya_ebpf::{
macros::{map, sk_msg},
maps::SockHash,
programs::SkMsgContext,
};
use aya_log_ebpf::info;
use {{crate_name}}_common::SockKey;

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.

Generating cargo doc for both userland and eBPF program

3 participants