-
Notifications
You must be signed in to change notification settings - Fork 154
Tokio console feature in the Deploy CI job #3958
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
Conversation
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.
Pull request overview
This PR adds support for the tokio-console feature in the Deploy CI job, following up on #3950. It improves the CI workflow UI by introducing a predefined feature dropdown and a custom features input field for better flexibility in future development.
Key changes:
- Updated GitHub workflow to support predefined feature selection (None, mimalloc-allocator, tokio-console) with an additional custom features field
- Added RUSTFLAGS build argument to Dockerfile with proper tokio_unstable flag handling for tokio-console
- Added
tokio-consolefeature definitions to multiple crate Cargo.toml files (alerter, autopilot, driver, orderbook, refunder, solvers) - Updated playground/Dockerfile to use the simplified
tokio-consolefeature flag instead ofobserve/tokio-console
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/deploy.yaml |
Enhanced workflow inputs with predefined feature dropdown and custom features field; added logic to prepare build features and set RUSTFLAGS for tokio-console |
Dockerfile |
Added RUSTFLAGS build argument and passed it to cargo build command |
playground/Dockerfile |
Changed feature flag from observe/tokio-console to tokio-console for all package builds |
crates/alerter/Cargo.toml |
Added tokio-console feature that enables observe/tokio-console |
crates/autopilot/Cargo.toml |
Added tokio-console feature that enables observe/tokio-console |
crates/driver/Cargo.toml |
Added tokio-console feature that enables observe/tokio-console |
crates/orderbook/Cargo.toml |
Added tokio-console feature that enables observe/tokio-console |
crates/refunder/Cargo.toml |
Added tokio-console feature that enables observe/tokio-console |
crates/solvers/Cargo.toml |
Added tokio-console feature that enables observe/tokio-console |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| COPY ./crates/observe/ ./crates/observe | ||
| COPY ./crates/order-validation/ ./crates/order-validation | ||
| RUN CARGO_PROFILE_RELEASE_DEBUG=1 cargo build --release --features observe/tokio-console --package shared | ||
| RUN CARGO_PROFILE_RELEASE_DEBUG=1 cargo build --release --features tokio-console --package shared |
Copilot
AI
Dec 9, 2025
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.
The shared crate does not have a tokio-console feature defined in its Cargo.toml. Building with --features tokio-console --package shared will fail because this feature doesn't exist for the shared package.
You need to add the following to crates/shared/Cargo.toml:
[features]
# ... existing features ...
tokio-console = ["observe/tokio-console"]
jmg-duarte
left a comment
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.
LGTM but I'm worried about the custom field
MartinquaXD
left a comment
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.
Looks reasonable to me. I'd just like to see if new code in the action can be reduced a bit. Fine to merge if my understanding is not correct and this is indeed not possible (i.e. would lead to undesired outcomes).
.github/workflows/deploy.yaml
Outdated
| if [ -n "${{ github.event.inputs.custom_features }}" ]; then | ||
| if [ -n "$FEATURES" ]; then | ||
| FEATURES="$FEATURES ${{ github.event.inputs.custom_features }}" |
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.
This is also the right thing to do if FEATURES is empty, no? I believe we don't need the inner if and just do that unconditionally.
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 didn't get your comment. It appends custom features to the features if they are not empty.
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 mean if FEATURES is empty the line FEATURES="$FEATURES ${{ github.event.inputs.custom_features }}" would also do the desired thing (of making FEATURES equal to github.event.inputs.custom_features). So from my understanding this should be sufficient:
if [ -n "${{ github.event.inputs.custom_features }}" ]; then
FEATURES="$FEATURES ${{ github.event.inputs.custom_features }}"
fi
Actually applying the same logic just doing this (without any ifs) should also be fine:
FEATURES="$FEATURES ${{ github.event.inputs.custom_features }}"
If FEATURES is empty we prepend an empty string => no harm done. If github.event.inputs.custom_features is empty we append and empty string => no harm done.
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.
Your suggestion can end up with features being just a space, then "--features " will be passed to the Dockerfile.
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.
Claude suggested a shorter version. Updated the code b3387e6 (#3958)
Description
This is a follow-up to #3950, where the deploy CI job now supports builds with tokio-console enabled. Also, improves the CI UI, which now includes a list of options and an additional field with custom features, which should be helpful for future development.