Skip to content
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

Use cargo hakari to automate workspace-hack #2472

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Use cargo hakari to automate workspace-hack #2472

merged 1 commit into from
Jan 8, 2025

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Jan 7, 2025

This PR integrates workspace-hack (via cargo hakari), this fixes the huge space amplification that we had with rocksdb builds, and it building/checking individual crates becomes significantly faster after the initial build.

The cost of this:

  • Slightly longer initial build time (but I didn't really notice much difference on my machine)
  • I removed io-uring optional feature (easy to add back when we decide to give it another try) from rocksdb
  • After adding new third-party dependencies, make sure you run cargo hakari generate, don't worry, CI will catch if you didn't.

This is a non-destructive change, and running with cargo hakari disable should still work if this is needed.

Copy link

github-actions bot commented Jan 7, 2025

Test Results

  7 files  ±0    7 suites  ±0   4m 18s ⏱️ -18s
 47 tests ±0   46 ✅ ±0  1 💤 ±0  0 ❌ ±0 
182 runs  ±0  179 ✅ ±0  3 💤 ±0  0 ❌ ±0 

Results for commit cd0b4e0. ± Comparison against base commit b506fc6.

♻️ This comment has been updated with latest results.

@@ -0,0 +1,2 @@
// A build script is required for cargo to consider build dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also include the license header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an empty file that must remain empty. I don't think license header adds any value to that.

@@ -0,0 +1 @@
// This is a stub lib.rs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this file

This PR integrates workspace-hack (via [cargo hakari](https://crates.io/crates/cargo-hakari)), this fixes the huge space amplification that we had with rocksdb builds, and it building/checking individual crates becomes significantly faster after the initial build.

The cost of this:
- Slightly longer initial build time (but I didn't really notice much difference on my machine)
- I removed io-uring optional feature (easy to add back when we decide to give it another try) from rocksdb
- After adding new third-party dependencies, make sure you run `cargo hakari generate`, don't worry, CI will catch if you didn't.

This is a non-destructive change, and running with `cargo hakari` disable should still work if this is needed.
@AhmedSoliman AhmedSoliman marked this pull request as ready for review January 8, 2025 09:59
Copy link
Contributor

@jackkleeman jackkleeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very very cool. Works for me locally, and I see no reason why this would impact CI as the tool is not needed at build time. Thank you!!

@pcholakov
Copy link
Contributor

pcholakov commented Jan 8, 2025

This is great, @AhmedSoliman! Works great for me as well.

On my machine, cargo build from a clean workspace takes about 5-6s longer than before:

cargo build  990.58s user 213.14s system 839% cpu 2:23.35 total

Compared to main this is completely negligible for the added benefit:

cargo build  899.85s user 201.60s system 798% cpu 2:17.90 total

I noticed that unrelated to your change, we no longer need the special-case target dir for test targets in the just file, as switching between cargo build / cargo run and cargo nextest no longer rebuilds the whole world 🎉 Since we build tests with --all-features that does require a rebuild, but switching between test and non-test targets isn't as expensive as it used to be though that's independent of the cargo hakari change. I'll open a PR with that.

@AhmedSoliman AhmedSoliman merged commit e6d1917 into main Jan 8, 2025
29 checks passed
@AhmedSoliman AhmedSoliman deleted the pr2472 branch January 8, 2025 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants