-
Notifications
You must be signed in to change notification settings - Fork 8
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
Release 0.2.0 #44
Release 0.2.0 #44
Conversation
b5678e7
to
5240815
Compare
Signed-off-by: Daiki Ueno <[email protected]>
To make all crates use the same version of dependencies, this switches to using the workspace.dependencies feature. This also updates the following dependencies to make packaging easier: - page_size to 0.6 - probe version to 0.5 - toml to 0.7 - serde_with to 3 Signed-off-by: Daiki Ueno <[email protected]>
Signed-off-by: Daiki Ueno <[email protected]>
5240815
to
64ab6d9
Compare
@t184256 could you check this as well? |
serde.workspace = true | ||
serde_cbor.workspace = true | ||
time = { workspace = true, features = ["formatting", "local-offset", "macros"] } | ||
tokio = { workspace = true, features = ["fs", "io-util", "signal"] } | ||
tokio-uring = { version = "0.4", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to single it out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the documentation, optional dependencies cannot be declared in workspace.dependencies.
"crypto-auditing", | ||
"event-broker", | ||
"log-parser" | ||
] | ||
resolver = "2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I know it's not changed in here, but I'd still like to ask)
Features for target-specific dependencies are not enabled if the target is not currently being built.
is either a rather confusing feature description or a rather confusing feature. My testing seems to support the idea that the enabled feature set becomes the superset of the packages' features of the invocation. That suggests that 1. cargo build
result would differ from make
, 2. library featureset depends on whichever application target was built first, etc.
a. Should we strive for more isolation, e.g., by building the library first in a separate invocation?
b. Given that it still doesn't guarantee we've specified the features correctly, should we, maybe, give up and define just the all-encompassing superset at the top level and at least have the consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I get it right, but I can for example turn off io_uring in the agent while keeping libsystemd in the event-broker with make RELEASE=1 CARGO_ARGS="--no-default-features --features=libsystemd"
. Isn't that sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(clarified off-PR. We don't have builds that go "one app + library" and library inheriting a featureset of whatever app is built first, as I've mistakenly thought after seeing four app targets in Makefile. We'll either build the entire workspace at once with the union-of-all featureset or build subprojects one-by-one with their individual featuresets. As long as we test all of the approaches we use, this is fine.)
crypto-auditing.workspace = true | ||
futures.workspace = true | ||
inotify.workspace = true | ||
libsystemd = { version = "0.7", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, optional dependency.
agent/src/bpf/vmlinux.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- can it be trimmed down?
- can it be pulled in from a separate crate, with its own repo, release cycle, versioning..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say (1) is not practical, because functions in bpf-helpers are also defined there and have tight dependencies on other part of the header. (2) might be an interesting idea, though I would defer it to "cargo build" time, rather than "cargo package".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I managed to add a logic to generate the header file on the fly.
64ab6d9
to
b6fa8f7
Compare
This adds a logic to generate "vmlinux.h" in build.rs, needed for compiling BPF programs. If vmlinux.h is supplied by the makefile, it will be copied to the build directory; otherwise bpftool will be used to extract it from the running kernel. Signed-off-by: Daiki Ueno <[email protected]>
Signed-off-by: Daiki Ueno <[email protected]>
b6fa8f7
to
0177e4a
Compare
Thanks for the review! |
This prepares all changes needed to publish 0.2.0. Note that #43 needs to be merged first.