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

Replace toml dependency with toml_edit to preserve comments #407

Merged
merged 8 commits into from Oct 1, 2023

Conversation

danjl1100
Copy link
Contributor

@danjl1100 danjl1100 commented Sep 30, 2023

Motivation

As discussed in #394, crane currently uses the toml crate dependency in crane-resolve-workspace-inheritance.rs in order to merge the workspace root Cargo.toml with the current crate's Cargo.toml. This script is run on all git dependencies of the built crate.

One unfortunate side effect of using the toml crate is that all comments are removed from Cargo.toml. This is an issue if proc macros or build scripts are expecting to read non-TOML metadata from Cargo.toml (e.g. document-features used by some embedded libraries, such as embassy-stmf32)

Solution

This PR adapts the existing crane-resolve-workspace-inheritance.rs script to use toml_edit instead of the toml crate, to preserve comments.

Remarks

  • There are two kinds of Tables in toml_edit, so some complexity (e.g. trait TableLikeExt) is needed to merge any combination of tables.
  • The TOML key/value formatting cleanup is not strictly required, but I included this to keep the current smoke test mostly unchanged. One possible alternative is to use the toml crate as a dev-dependency, to only check the TOML is equal and avoid string equality; then the fmt module could be removed.

I tested this on my minimal example from #394, and it appears to work correctly. (reference danjl1100/example_uses_dep_using_document_features@7cc5174)

Checklist

  • added tests to verify new behavior
  • added an example template or updated an existing one
  • updated docs/API.md (or general documentation) with changes
  • updated CHANGELOG.md

@danjl1100 danjl1100 marked this pull request as ready for review September 30, 2023 21:40
Copy link
Owner

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! Direction looks good, just have a few minor questions

CHANGELOG.md Outdated Show resolved Hide resolved
@ipetkov ipetkov enabled auto-merge (squash) October 1, 2023 20:48
@ipetkov ipetkov merged commit 03e442f into ipetkov:master Oct 1, 2023
7 checks passed
@danjl1100 danjl1100 deleted the toml_edit branch October 2, 2023 18:49
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.

None yet

2 participants