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

Use default-members in Cargo workspace #58

Closed
wants to merge 1 commit into from

Conversation

dave-tucker
Copy link
Member

Adds bpf code to the workspace and is excluded by default.
This allows for all deps to be managed in Cargo.lock and for cargo
update to work as expected from the root.
Similarly, rustfmt and clippy can operate on the whole workspace.

Signed-off-by: Dave Tucker [email protected]

Cargo.toml Outdated
[profile.dev]
panic = "abort"
lto = true
rpath = false
Copy link
Contributor

Choose a reason for hiding this comment

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

rpath can be removed I think?

@@ -1,3 +1,4 @@
{
"rust-analyzer.linkedProjects": ["Cargo.toml", "{{project-name}}-ebpf/Cargo.toml"]
"rust-analyzer.checkOnSave.allTargets": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to set this to false?

Copy link
Member Author

Choose a reason for hiding this comment

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

because it suppresses a really annoying "cannot find module for test" message in RA - which happens because it's attempting to analyze the bpf crates as x86_64. There isn't a way (yet) to say these crates are only supported on these subset of targets as far as RA is concerned.

@@ -1,3 +1,4 @@
{
"rust-analyzer.linkedProjects": ["Cargo.toml", "{{project-name}}-ebpf/Cargo.toml"]
"rust-analyzer.checkOnSave.allTargets": false,
"rust-analyzer.checkOnSave.command": "clippy"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this not the default?

Copy link

Choose a reason for hiding this comment

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

Screenshot 2022-09-10 at 15 54 49

@davibe
Copy link

davibe commented Sep 21, 2022

@dave-tucker I tested this a bit.

This syntax

profile.dev.package.{{project-name}}-ebpf]

only affects the single module in the workspace but not its dependencies.

This means for example that a dependency of {{project-name}} like aya-log-ebpf will not get the options applied, and the build will not work.

I got my test project to work by adding profile.dev.package."*"] and repeating the options in this new section. However this matches all deps of all member so it's not a good solution.

I have seen that cargo target. syntax supports target.'cfg(...)', which could allow to match things based on the target architecture, but I did not find a way to use it with profile.

Adds bpf code to the workspace and is excluded by default.
This allows for all deps to be managed in Cargo.lock and for cargo
update to work as expected from the root.
Similarly, rustfmt and clippy can operate on the whole workspace.

Signed-off-by: Dave Tucker <[email protected]>
@dave-tucker
Copy link
Member Author

@dave-tucker I tested this a bit.

This syntax

profile.dev.package.{{project-name}}-ebpf]

only affects the single module in the workspace but not its dependencies.

Yeah this is unfortunate. Feel free to test again - this has now been added into the .cargo/config.toml so it should affect everything compiled for the BPF targets. Longer term we'll want something like rust-lang/cargo#4897

@alessandrod
Copy link
Contributor

@dave-tucker I tested this a bit.
This syntax
profile.dev.package.{{project-name}}-ebpf]
only affects the single module in the workspace but not its dependencies.

Yeah this is unfortunate. Feel free to test again - this has now been added into the .cargo/config.toml so it should affect everything compiled for the BPF targets. Longer term we'll want something like rust-lang/cargo#4897

This doesn’t seem like a great idea, it breaks profiles and at the moment is only needed for debug support, which we don’t support yet. I don’t think we should do this as part of this PR.

@alessandrod
Copy link
Contributor

@dave-tucker I tested this a bit.
This syntax
profile.dev.package.{{project-name}}-ebpf]
only affects the single module in the workspace but not its dependencies.

Yeah this is unfortunate. Feel free to test again - this has now been added into the .cargo/config.toml so it should affect everything compiled for the BPF targets. Longer term we'll want something like rust-lang/cargo#4897

This doesn’t seem like a great idea, it breaks profiles and at the moment is only needed for debug support, which we don’t support yet. I don’t think we should do this as part of this PR.

Never mind I missed the other comment about cfg syntax not working for profiles. Omg so many papercuts

"-C", "lto=yes",
"-C", "embed-bitcode=yes",
"-C", "debuginfo=2",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not turn this on now. It’ll make building slower for no reason

@tamird
Copy link
Member

tamird commented Oct 10, 2024

Consider rebasing this after #123.

@tamird
Copy link
Member

tamird commented Oct 10, 2024

This will be closed by #124.

@tamird tamird closed this in #124 Oct 11, 2024
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.

4 participants