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

Replace xtask builds with build scripts #124

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Replace xtask builds with build scripts #124

merged 4 commits into from
Oct 11, 2024

Conversation

tamird
Copy link
Member

@tamird tamird commented Oct 10, 2024

See individual commits.

Closes #22.
Closes #58.
Closes #96.
Closes #110.

Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

I was hoping to test this out - IIRC the main reason we used xtask was because build.rs could cause the rust-language-server to go bananas 🍌 (rebuilding the build script on every change).

After correcting the path to the bytecode, cargo build fails as follows:

$ cargo build       
   --> test-buildscript-hotness/src/main.rs:49:10
    |
48  | /     program.attach(&opt.iface, XdpFlags::default())
49  | |         .context("failed to attach the XDP program with default flags - try changing XdpFlags::default() to XdpFlags::SKB_MOD...
    | |         -^^^^^^^ method cannot be called on `Result<XdpLinkId, ProgramError>` due to unsatisfied trait bounds
    | |_________|
    |
    |
   ::: /home/dave/.cargo/registry/src/index.crates.io-6f17d22bba15001f/aya-0.13.0/src/programs/mod.rs:132:1
    |
132 |   pub enum ProgramError {
    |   --------------------- doesn't satisfy `_: StdError`
    |
   ::: /home/dave/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:527:1
    |
527 |   pub enum Result<T, E> {
    |   --------------------- doesn't satisfy `_: Context<XdpLinkId, ProgramError>`
    |
    = note: the following trait bounds were not satisfied:
            `aya::programs::ProgramError: anyhow::context::ext::StdError`
            which is required by `Result<XdpLinkId, aya::programs::ProgramError>: anyhow::Context<XdpLinkId, aya::programs::ProgramError>`

error[E0277]: `?` couldn't convert the error to `anyhow::Error`
  --> test-buildscript-hotness/src/main.rs:38:8
   |
38 |     )))?;
   |        ^ the trait `From<EbpfError>` is not implemented for `anyhow::Error`, which is required by `Result<(), _>: FromResidual<Result<Infallible, EbpfError>>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `FromResidual<R>`:
             <Result<T, F> as FromResidual<Result<Infallible, E>>>
             <Result<T, F> as FromResidual<Yeet<E>>>
   = note: required for `Result<(), anyhow::Error>` to implement `FromResidual<Result<Infallible, EbpfError>>`

error[E0277]: `?` couldn't convert the error to `anyhow::Error`
  --> test-buildscript-hotness/src/main.rs:46:20
   |
46 |         .try_into()?;
   |                    ^ the trait `From<aya::programs::ProgramError>` is not implemented for `anyhow::Error`, which is required by `Result<(), _>: FromResidual<Result<Infallible, aya::programs::ProgramError>>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `FromResidual<R>`:
             <Result<T, F> as FromResidual<Result<Infallible, E>>>
             <Result<T, F> as FromResidual<Yeet<E>>>
   = note: required for `Result<(), anyhow::Error>` to implement `FromResidual<Result<Infallible, aya::programs::ProgramError>>`

error[E0277]: `?` couldn't convert the error to `anyhow::Error`
  --> test-buildscript-hotness/src/main.rs:47:19
   |
47 |     program.load()?;
   |                   ^ the trait `From<aya::programs::ProgramError>` is not implemented for `anyhow::Error`, which is required by `Result<(), _>: FromResidual<Result<Infallible, aya::programs::ProgramError>>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `FromResidual<R>`:
             <Result<T, F> as FromResidual<Result<Infallible, E>>>
             <Result<T, F> as FromResidual<Yeet<E>>>
   = note: required for `Result<(), anyhow::Error>` to implement `FromResidual<Result<Infallible, aya::programs::ProgramError>>`

error[E0277]: `?` couldn't convert the error to `anyhow::Error`
  --> test-buildscript-hotness/src/main.rs:52:27
   |
52 |     signal::ctrl_c().await?;
   |                           ^ the trait `From<std::io::Error>` is not implemented for `anyhow::Error`, which is required by `Result<(), _>: FromResidual<Result<Infallible, std::io::Error>>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `FromResidual<R>`:
             <Result<T, F> as FromResidual<Result<Infallible, E>>>
             <Result<T, F> as FromResidual<Yeet<E>>>
   = note: required for `Result<(), anyhow::Error>` to implement `FromResidual<Result<Infallible, std::io::Error>>`

warning: unused import: `anyhow::Context`
 --> test-buildscript-hotness/src/main.rs:1:5
  |
1 | use anyhow::Context as _;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

Some errors have detailed explanations: E0277, E0599.
For more information about an error, try `rustc --explain E0277`.
warning: `test-buildscript-hotness` (bin "test-buildscript-hotness") generated 1 warning
error: could not compile `test-buildscript-hotness` (bin "test-buildscript-hotness") due to 5 previous errors; 1 warning emitted

This might be something unique to my environment, but I'd like to be sure before we merge it.
You should be able to reproduce with:

cargo generate -g https://github.com/aya-rs/aya-template -b build-script-hotness

{{project-name}}/src/main.rs Outdated Show resolved Hide resolved
{% if program_types_with_opts contains program_type -%}
use clap::Parser;
{% endif -%}
use log::{info, warn, debug};

#[rustfmt::skip]
Copy link
Member

Choose a reason for hiding this comment

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

👀 what does rustfmt do here that warrants skipping?

Copy link
Member Author

Choose a reason for hiding this comment

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

It tries to reorder the imports in the sk_msg case because use {{crate_name}}_common::SockKey;( use test_common::SockKey;`) sorts after these items.

@tamird tamird force-pushed the build-script-hotness branch from 094b296 to 035cf5d Compare October 11, 2024 09:41
@tamird
Copy link
Member Author

tamird commented Oct 11, 2024

This might be something unique to my environment, but I'd like to be sure before we merge it. You should be able to reproduce with:

cargo generate -g https://github.com/aya-rs/aya-template -b build-script-hotness

is that the invocation you used? Doesn't that pull this repo at HEAD rather than this PR?

@tamird
Copy link
Member Author

tamird commented Oct 11, 2024

Ah, I think you are running into this caveat because we're now building anyhow with no_std and you must be using Rust < 1.81.

@tamird tamird force-pushed the build-script-hotness branch from a7bdbe1 to 5d645df Compare October 11, 2024 09:54
@tamird
Copy link
Member Author

tamird commented Oct 11, 2024

Should work for you now

@tamird tamird force-pushed the build-script-hotness branch 4 times, most recently from 1a7d70a to 925479b Compare October 11, 2024 15:37
@tamird tamird requested a review from dave-tucker October 11, 2024 15:41
@tamird tamird force-pushed the build-script-hotness branch 2 times, most recently from 20787f0 to 66ac58b Compare October 11, 2024 16:04
Adapt aya-rs/aya@3d463a3 and subsequent work
to the template. This has worked very well for us in the main project,
and our users should get the same hotness.

Note that xtask is still used for running, as it is in the main project.
@tamird tamird force-pushed the build-script-hotness branch from 66ac58b to 9b76a3a Compare October 11, 2024 16:07
@tamird tamird changed the title Replace xtask with build scripts Replace xtask builds with build scripts Oct 11, 2024
Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Works for me 🐧

@tamird tamird merged commit b6ecbb8 into main Oct 11, 2024
39 checks passed
@tamird tamird deleted the build-script-hotness branch October 11, 2024 16:43
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