bcs: clean deps, reduce CI clean build time#14
Conversation
`bcs` only uses proc-macro serde derives in tests. We can reduce compile times by using `serde_core` directly, which only exports the core serde traits.
|
Would be nice to see it merged, but latest commit on main branch is 3 years ago... |
|
cc @ma2bd, you guys still using |
|
Interesting. How much time was spent in @bmwill what do you think? |
|
@phlip9 Its been a while! Yeah i'd be supportive of a change like this. I think 100% of what we get from thiserror can be rather trivially implemented explicitly (and tools like claude made maintaining that not that much of a lift) |
|
Just to chime in, if you decide to keep thiserror, could we bump it to v2 + But Imho what @phlip9 did looks pretty good to me |
Before:
```
$ cargo tree --edges build,normal
bcs v0.1.6 (/home/phlip9/dev/bcs)
├── serde v1.0.228
│ ├── serde_core v1.0.228
│ └── serde_derive v1.0.228 (proc-macro)
│ ├── proc-macro2 v1.0.106
│ │ └── unicode-ident v1.0.24
│ ├── quote v1.0.45
│ │ └── proc-macro2 v1.0.106 (*)
│ └── syn v2.0.117
│ ├── proc-macro2 v1.0.106 (*)
│ ├── quote v1.0.45 (*)
│ └── unicode-ident v1.0.24
└── thiserror v1.0.69
└── thiserror-impl v1.0.69 (proc-macro)
├── proc-macro2 v1.0.106 (*)
├── quote v1.0.45 (*)
└── syn v2.0.117 (*)
$ rm -rf /tmp/cargo-target && taskset -c 3 cargo build --target-dir /tmp/cargo-target --lib
Finished dev [unoptimized + debuginfo] target(s) in 7.89s
```
After:
```
$ cargo tree --edges build,normal
Updating crates.io index
bcs v0.1.6 (/home/phlip9/dev/bcs)
└── serde_core v1.0.228
$ rm -rf /tmp/cargo-target && taskset -c 3 cargo build --target-dir /tmp/cargo-target --lib
Compiling serde_core v1.0.228
Compiling bcs v0.1.6 (/home/phlip9/dev/bcs)
Finished dev [unoptimized + debuginfo] target(s) in 1.38s
```
In theory we should be able to support an MSRV of `1.56`. Unfortunately this
entry in `serde_core`'s Cargo.toml seems to confuse Cargo into adding
`serde_derive` into the lock file, which only has a compatible MSRV of `1.61`.
```toml
[target.'cfg(any())'.dependencies]
serde_derive = { version = "=1.0.220", path = "../serde_derive" }
```
4837312 to
6ca5804
Compare
|
Took the liberty of fixing up CI. Unfortunately due to some cargo dependency resolution quirks, I've had to raise the MSRV from 1.56 -> 1.61. In theory 1.56 should work. Unfortunately this entry in [target.'cfg(any())'.dependencies]
serde_derive = { version = "=1.0.220", path = "../serde_derive" }I've also committed a working The rust-version -aware resolver is also completely useless... so the fixups need to be done manually: |
Thanks! |
|
It looks like you guys are getting closer to merge that! @bmwill is there anything else important left that needs to be addressed? Would it be possible to publish new version after this is merged? |
|
@phlip9 This was published in version |

Changes
serde_coreDescription
Hey!
I noticed
bcson the critical path of abuild.rsscript in CI, so I took a pass at fixing that. After some slight refactoring to (1) removethiserrorand (2) useserde_coretraits directly, the "1 vCPU in CI" clean build time is down ~5.7x, without many downsides:Before:
After: