Build the workspace in the flake #2200
Conversation
|
Bencher Report| Branch | flake-build-workspace |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | microseconds (µs) |
|---|---|---|
| fibonacci 10 | 📈 view plot 🚷 view threshold | 370.77 µs |
| foldl arrays 50 | 📈 view plot 🚷 view threshold | 515.06 µs |
| foldl arrays 500 | 📈 view plot 🚷 view threshold | 4,913.10 µs |
| foldr strings 50 | 📈 view plot 🚷 view threshold | 5,783.50 µs |
| foldr strings 500 | 📈 view plot 🚷 view threshold | 59,313.00 µs |
| generate normal 250 | 📈 view plot 🚷 view threshold | 49,394.00 µs |
| generate normal 50 | 📈 view plot 🚷 view threshold | 1,955.20 µs |
| generate normal unchecked 1000 | 📈 view plot 🚷 view threshold | 2,988.50 µs |
| generate normal unchecked 200 | 📈 view plot 🚷 view threshold | 606.84 µs |
| pidigits 100 | 📈 view plot 🚷 view threshold | 2,706.90 µs |
| pipe normal 20 | 📈 view plot 🚷 view threshold | 929.16 µs |
| pipe normal 200 | 📈 view plot 🚷 view threshold | 9,110.00 µs |
| product 30 | 📈 view plot 🚷 view threshold | 324.93 µs |
| scalar 10 | 📈 view plot 🚷 view threshold | 755.90 µs |
| sum 30 | 📈 view plot 🚷 view threshold | 321.63 µs |
jneem
force-pushed
the
flake-build-workspace
branch
from
9c55b63 to
e65a1de
Compare
There was a problem hiding this comment.
This looks good. Do we have any way of measuring, quantitatively or qualitatively the effect of this PR? I always find it a bit hard to know if this has any positive or negative effects, given that the CI run time is also impacted by previous caching. We still seem to build a lot of things in the action, but is it less or more than before? 🤷♂️
| buildStaticPackage = { pnameSuffix, cargoPackage, extraBuildArgs ? "", extraArgs ? { } }: | ||
| (buildPackage { | ||
| inherit pnameSuffix cargoPackage; | ||
| buildStaticWorkspace = { pnameSuffix, extraBuildArgs ? "", extraArgs ? { } }: |
There was a problem hiding this comment.
It's a detail but the ability to decouple pnameSuffix from the original cargo package name had a small upside, which is that the derivations would exhibit more specific names (nickel-lang-cli-pkg for example, instead of just nickel-lang-cli). It's not very important, but was reason for sometimes having a pnameSuffix that doesn't match precisely the package name.
There was a problem hiding this comment.
I'm not sure I understand: we don't use cargo package names anymore, so why would we need to decouple them from suffices? We can still have derivations with more specific names (like nickel-lang-pkg vs nickel-lang)
There was a problem hiding this comment.
Sure, my point I guess is that as it is, this PR will show nickel-lang-cli> in the logs for both nickel-lang-cli and nickel-lang-cli-pkg, while it didn't use to be the case, is that correct? I agree that we don't really care about the pnameSuffix mechanism in particular.
There was a problem hiding this comment.
Good point. I think the latest version is using pnameSuffix correctly now.
Good question, I'll see if I can measure this somehow |
|
I can confirm that this approach reduces the size of the cached cargo dependencies. Previously, I had After this patch, I have So, 850M -> 580M. |
This changes the nix packages so that they build the whole workspace instead of going package-by-package. Because of the way feature unification works in cargo, this should reduce the number of times we need to build dependencies.
As a consequence, this also runs more tests in CI:
nix flake checknow runs the tests for all crates, not just the few that we selected.It may be possible to get back to package-by-package builds in the future, since cargo would like to improve this on their end: https://rust-lang.github.io/rfcs/3692-feature-unification.html