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

Switch to tomling crate for reduced dependencies #56

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zeenix
Copy link
Contributor

@zeenix zeenix commented Jan 11, 2025

tomling crate was specifically created to provide a simple TOML parser with fewer dependencies and focus on Cargo.toml parsing.

Fixes #37.


Creating a draft PR first to get some input before I roll out the tomling release and then switching to released version in this PR.

@zeenix zeenix force-pushed the tomling branch 2 times, most recently from 3ec1e96 to b5d4f89 Compare January 11, 2025 21:12
Copy link
Owner

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Generally I'm fine to switch. However, I have some questions before :)

  1. Did you make any compile time comparisons? What is the difference?
  2. How good is tomling tested? Did you run any tests against some larger set of toml files to ensure it doesn't crash etc?

.chain(dev_deps.into_iter().flat_map(|deps| deps.iter()))
.collect::<Vec<_>>();
// Ensure renames (i-e deps with `package` key) are listed the last.
deps.sort_by(|(_, a), (_, b)| match (a.package(), b.package()) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the using code relies on this. I added this after a test case was failing because it was getting the name rather than the rename (or was it the other way around? 🤔).

`tomling` crate was specifically created to provide a simple TOML
parser with fewer dependencies and focus on Cargo.toml parsing.

Fixes bkchr#37.
@zeenix
Copy link
Contributor Author

zeenix commented Jan 13, 2025

However, I have some questions before :)

Fair. :)

  1. Did you make any compile time comparisons? What is the difference?

Right now there is no noticeable difference tbh. However:

  1. I've not yet applied any optimizations to tomling. There are some rather low-hanging ones, such as parsing the string as bytes (which in other projects have yielded huge improvements) so I'm expecting this to improve.
  2. The release binary size (with LTO enabled), already show a slight improvement (around 7KB in busd).

I understand completely if you'd like to wait till tomling brings real significant improvements before switching though.

2. How good is tomling tested? Did you run any tests against some larger set of toml files to ensure it doesn't crash etc?

So we've added the amazing toml-test-harness to our test/CI, which tests all kinds of corner cases etc. While we don't have any crashes, since we're not yet 100% spec-compliant (e.g string escaping isn't supported yet), not all tests pass and hence explicitly ignored.

test result: ok. 399 passed; 0 failed; 165 ignored; 0 measured; 0 filtered out; finished in 0.01s

To save you some math, that's 70% coverage. Other than that, we also have fuzzing enabled (which is why I'm fairly confident we don't crash on any input) and some other test cases based on real world Cago.toml files (currently zbus and tokio). The test case of proc-macro-crate also help here.

Again, if you'd like to wait for full spec compliance, I understand completely.

@bkchr
Copy link
Owner

bkchr commented Jan 13, 2025

Right now there is no noticeable difference tbh. However:

I mean this crate should compile faster? That being the entire point of using a slimmer dependency?

@zeenix
Copy link
Contributor Author

zeenix commented Jan 13, 2025

Right now there is no noticeable difference tbh. However:

I mean this crate should compile faster?

It should, yes. :) However, at the moment, it doesn't for the reasons I mentioned. As I wrote, I understand if you want to wait till that is the case before merging. I'm hopeful it will still be soon.

That being the entire point of using a slimmer dependency?

My impression was that it was mainly about the binary size (at least for me that was the only motivation) but yeah, this would be one of the 2 benefits in the end.

Another benefit would be "perceived" compile speed. People often look at the number of deps being downloaded, instead of actual compile time to judge Rust projects. It's super silly, I know but it is what it is. 🤷

Anyway, let me know if you want to wait.

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.

Consider switching back from toml_edit to basic-toml
2 participants