refactor(s2n-quic-events): Rename s2n-quic-events to s2n-events #2780
Conversation
| - name: test | ||
| working-directory: ./tools/s2n-events | ||
| run: cargo test |
There was a problem hiding this comment.
There currently aren't any tests in s2n-events, but I plan to add some. And this will ensure that s2n-events is able to build successfully for now.
There was a problem hiding this comment.
Did we just never run clippy on this crate? Seems kind of odd. Maybe because it's all generated code we don't actually care about linting.
There was a problem hiding this comment.
Ahh it looks like clippy runs on every workspace it can find. Since s2n-quic-events was in the normal s2n-quic workspace, clippy probably was running before. Good catch!
I updated s2n-events to make it its own workspace, so now it will be discovered as a workspace and clippy will find it. I confirmed that it's listed in the workspaces now:
❯ find . -maxdepth 4 -name Cargo.toml -not -path './tools/xdp/ebpf/*' -exec grep -l '^\[workspace\]' {} \; | xargs -n1 dirname | sort | jq -R | jq -sc
[ ... "./tools/s2n-events","./tools/udp-attack","./tools/xdp"]
This reverts commit 830c34b.
| sed -i '/xdp/d' quic/s2n-quic-qns/Cargo.toml; \ | ||
| sed -i '/xdp/d' quic/s2n-quic/Cargo.toml; \ | ||
| rm -rf quic/s2n-quic-bench quic/s2n-quic-events quic/s2n-quic-sim quic/s2n-quic-tests | ||
| rm -rf quic/s2n-quic-bench quic/s2n-quic-sim quic/s2n-quic-tests |
There was a problem hiding this comment.
The quic directory is copied above, and s2n-quic-events is being deleted here. Now that s2n-events isn't in quic, it no longer needs to be deleted.
|
|
||
| - name: Run cargo llvm-cov | ||
| run: cargo llvm-cov --html --no-fail-fast --workspace --exclude s2n-quic-qns --exclude s2n-quic-events --all-features | ||
| run: cargo llvm-cov --html --no-fail-fast --workspace --exclude s2n-quic-qns --all-features |
There was a problem hiding this comment.
s2n-events is no longer in the s2n-quic workspace, so it no longer needs to be excluded:
Lines 13 to 16 in 8d7fd3e
| @@ -65,9 +65,12 @@ impl EventInfo<'_> { | |||
| crate_name: "s2n_quic", | |||
| input_path: concat!( | |||
| env!("CARGO_MANIFEST_DIR"), | |||
| "/../s2n-quic-core/events/**/*.rs" | |||
There was a problem hiding this comment.
Longer term we should have the program take these as arguments and then have the caller pass them
There was a problem hiding this comment.
I was thinking of turning s2n-events into a library that exposed Output/everything else that's needed to generate an events system that this program currently uses. Then s2n-quic and s2n-tls could each own a program like this with a dependency on s2n-events.
But maybe keeping it as a program that's shared between s2n-quic and s2n-tls would work too, and you'd just configure everything with arguments to it. It might be less flexible, like you couldn't define custom tracing subscribers and stuff. But maybe you could just define that stuff in the shared program and select between them with arguments 🤔
.github/workflows/ci.yml
Outdated
| run: cargo test | ||
|
|
||
| # ensures the event codegen is up to date | ||
| events: |
There was a problem hiding this comment.
could this be combined into the new s2n-events above?
There was a problem hiding this comment.
They could be combined, but I was thinking of them as doing two different things:
- The
s2n-eventsCI job will build/test/format the crate that generates events systems, s2n-events. Currently this lives in the s2n-quic repo, but presumably s2n-events and this CI job could be moved to some other repo in the future. So I was considering this to be separate from s2n-quic specific stuff. - The
eventsCI job generates the events system specific to s2n-quic in order to make sure it remains up-to-date. This job would continue living in s2n-quic even if s2n-events is moved to another repo someday. Currently it just calls the program in s2n-events to generate the events, but I'm planning on moving that out of s2n-events and into something owned by s2n-quic in a future PR, so this will be completely separate.
I renamed the events job to generate-events to try and make the difference more clear. Let me know if you still think they should be combined though and I will do that!
| - name: test | ||
| working-directory: ./tools/s2n-events | ||
| run: cargo test |
There was a problem hiding this comment.
Did we just never run clippy on this crate? Seems kind of odd. Maybe because it's all generated code we don't actually care about linting.
4 participants
Resolved issues:
Related to #2768
Description of changes:
s2n-quic-events will eventually begin generating an events system for s2n-tls. In preparation for a more generic s2n-quic-events, this PR renames s2n-quic-events to s2n-events.
Additionally, s2n-quic-events isn't really being used directly as an actual component of s2n-quic (there are no dependencies on s2n-quic-events). Rather, s2n-quic-events is used as a tool to generate the events systems for s2n-quic and s2n-quic-dc. As such, this PR also places s2n-events outside of the s2n-quic workspace, and into the more aptly named
./toolsdirectory. Eventually, we might consider moving s2n-events into a separate repository since it will be shared by s2n-quic and s2n-tls.Call-outs:
To simplify the PR, I kept the s2n-quic and s2n-quic-dc code generation program inside of s2n-events. Ultimately, this will go in a separate package, so that s2n-quic and s2n-tls can both have programs to generate their own events systems. See the following for how I'm planning on having this work:
47adeee
Testing:
Since s2n-events is now outside of the s2n-quic workspace, this PR also adds a new CI job to build/format this crate.
This PR modifies the command to generate the s2n-quic/s2n-quic-dc events system in the CI test which checks to ensure that the events systems stay up-to-date. I added a new event to s2n-quic and made sure the test properly fails without committing the updated generated code.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.