build: finalize feature gating on component #4895
Conversation
Follow-up to #4892. During review of that PR, I noticed that `cargo run -p pcli` was emitting a lot of warnings about unused imports. Here I try to address those warnings, by adding more conditional imports based on the `component` feature.
Uses the usual `cargo check`, failing on warnings, and adds a custom target-dir for caching the check results alongside the usual debug/release targets. Doing so enables fast rechecks from local cache. Lots of disk space required: ~50GB for the `./target/check/` directory. This is why I'm not bolting it to CI right now. When cache is warm locally, the whole `just check` flow takes ~6s.
| use ibc_types::core::{channel::ChannelId, client::Height as IbcHeight}; | ||
|
|
||
| #[cfg(feature = "component")] | ||
| use ibc_types::core::channel::PortId; |
There was a problem hiding this comment.
This change makes me suspect I'm optimizing for the disabling warnings, rather than meaningfully structuring the imports.
| cfg_if::cfg_if! { | ||
| if #[cfg(feature="component")] { | ||
| pub mod component; | ||
| pub mod event; |
There was a problem hiding this comment.
We do not want to ever feature gate the events!! Pindexer wants to read these without pulling in cnidarium and rocksdb
There was a problem hiding this comment.
Fair point, we should align imports and not the exports. But I'm confused about why the proposed breakage isn't detected at build time: I tried cherry-picking this commit onto the dex PRs you reference (#4894) and I was able to build pindexer just fine. That's not the same as pindexer actually working, of course, but I'd expect mucking around with the event interface would break things early.
|
Closing as a distraction: I'll just continue to use |
Describe your changes
Follow-up to #4892. During review of that PR, I noticed that
cargo run -p pcliwas emitting a lot of warnings about unused imports. Here I try to address those warnings, by adding more conditional imports based on thecomponentfeature.Issue ticket number and link
#4892, #4885.
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:
Testing and review
I pushed up a script that I used locally to evaluate this behavior. You can run it via
just check. It'll take a while on first run, and create about 50GB of local cache, but after that it's snappy: ~6s. Running that same script on the main branch will show errors, which motivated this PR. I'm intentionally not adding that script to CI because of the caching requirements. We can run it opportunistically locally.