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

Always build eBPF; remove xtask #132

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Always build eBPF; remove xtask #132

merged 1 commit into from
Oct 31, 2024

Conversation

tamird
Copy link
Member

@tamird tamird commented Oct 30, 2024

No description provided.

@tamird tamird requested a review from alessandrod October 30, 2024 10:20
{{project-name}}-ebpf/build.rs Outdated Show resolved Hide resolved
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.

I think you can fix the current failure by putting

components = ["rust-src"]

into {{project-name}}-ebpf/rust-toolchain.toml

@vadorovsky
Copy link
Member

I think you can fix the current failure by putting

components = ["rust-src"]

into {{project-name}}-ebpf/rust-toolchain.toml

nvm, we are installing rust-src in the Github workflow yaml, so that's not the issue. Something more fishy is going in with clippy.

I still think that putting it into rust-toolchain.toml would be good, so people don't have to install rust-src by hand

@tamird
Copy link
Member Author

tamird commented Oct 30, 2024

I think you can fix the current failure by putting

components = ["rust-src"]

into {{project-name}}-ebpf/rust-toolchain.toml

nvm, we are installing rust-src in the Github workflow yaml, so that's not the issue. Something more fishy is going in with clippy.

As always, cargo-in-cargo issue. Should be fixed now.

I still think that putting it into rust-toolchain.toml would be good, so people don't have to install rust-src by hand

Done.

Use `cargo build`, `cargo check`, etc. as normal. Run your program with:

```shell
cargo run --release --config 'target."cfg(all())".runner="sudo -E"'
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we set this in config.toml? It's what I do for my projects

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like that as a default. It should be opt in to invoke sudo.

Copy link
Member

@vadorovsky vadorovsky Oct 30, 2024

Choose a reason for hiding this comment

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

From one hand I find this command quite annoying - the syntax looks really weird.

From the other hand, I do agree that calling sudo should be opt in. Maybe someone wants to use doas or something else.

Only if cargo supported something env var like CARGO_RUNNER, so users could just do export CARGO_RUNNER="sudo -E", which looks less wild.

At the end I think I have a slight preference towards not defaulting.

Copy link
Member Author

Choose a reason for hiding this comment

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

If there are still concerns with this I'll address them in a follow-up.

{{project-name}}/build.rs Outdated Show resolved Hide resolved
@tamird tamird force-pushed the remove-xtask branch 3 times, most recently from 984e537 to a5bf57d Compare October 30, 2024 18:23
@tamird tamird requested a review from vadorovsky October 30, 2024 19:41
@tamird tamird force-pushed the remove-xtask branch 8 times, most recently from 90bd7fe to 270aa0f Compare October 31, 2024 14:24
@tamird tamird merged commit b1d6fb3 into main Oct 31, 2024
22 checks passed
@tamird tamird deleted the remove-xtask branch October 31, 2024 14:54
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