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

Separate rustfmt into multiple crates #3998

Merged
merged 18 commits into from
Jan 5, 2020

Conversation

topecongiro
Copy link
Contributor

This PR separates rustfmt into multiple crates:

  • rustfmt-lib
  • rustfmt-emitter
  • rustfmt-configuration

Since rustfmt is registered as a workspace member in the rustc repository, we cannot directly use the workspace in rustfmt. As such, we put the above crates under the rustfmt-core directory.

Unnecessarily amount of commits shows my struggle trying to publish workspace members to crates.io and to patch ignore crate 🤷‍♂️ Sorry about that!

@calebcartwright
Copy link
Member

Awesome! One big step closer to rustfmt 2.x!

Regarding the patched version of ignore, is that related to the crossbeam issue and do you anticipate using the patched version long term or just until ignore itself is updated (ref BurntSushi/ripgrep#1423)?

shell: cmd
- name: 'test rustfmt-core ignored'
run: cargo test --manifest-path rustfmt-core/Cargo.toml -- --ignored
shell: cmd
Copy link
Member

@calebcartwright calebcartwright Jan 5, 2020

Choose a reason for hiding this comment

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

Another enhacement we could consider making with GHA in the future would be to set up some additional workflows that have directory/crate specific triggers. As an example, if the only changes are to the emitter crate, our CI will only have to build the emitter crate

@@ -0,0 +1,53 @@
[package]
name = "rustfmt_lib"
version = "1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

What'd you have in mind for versioning strategy between these different crates and the main rustfmt bin folks will get via rustup? Will they each be versioned independently, should the lib and bin versions be kepty in sync, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The crates in rustfmt-core will be versioned differently from binaries (e.g., rustfmt 3.0 may depend on rustfmt-core 4.0, rustfmt-emitter 2.0, and rustfmt-configuration 3.0).

license = "Apache-2.0/MIT"
repository = "https://github.com/rust-lang/rustfmt"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
Copy link
Member

Choose a reason for hiding this comment

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

Can probably remove the cargo-generated lines like this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right. Good catch, thank you!

@topecongiro
Copy link
Contributor Author

Regarding the patched version of ignore, is that related to the crossbeam issue and do you anticipate using the patched version long term or just until ignore itself is updated (ref BurntSushi/ripgrep#1423)?

Yes, using rustfmt-ignore is a temporal workaround.

@topecongiro topecongiro merged commit 6779a94 into rust-lang:master Jan 5, 2020
@topecongiro topecongiro deleted the rustfmt-core branch January 5, 2020 07:41
dtolnay added a commit to dtolnay/syn that referenced this pull request Jan 5, 2020
@BurntSushi
Copy link
Member

FYI, ignore 0.4.11 on crates.io now uses crossbeam-channel 0.4.

@calebcartwright
Copy link
Member

Awesome, thanks @BurntSushi!

@karyon
Copy link
Contributor

karyon commented Oct 28, 2021

backport not needed (I think)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants