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

remove derive_more #60

Merged
merged 5 commits into from
May 13, 2024
Merged

Conversation

xd009642
Copy link
Contributor

@xd009642 xd009642 commented May 10, 2024

First appologies for the size of this PR I can break it up if desired into a PR per file or something less giant 😅

This PR removes derives_more and thus the dependency on syn/quote etc. Both versions of syn take approximately 25-30s for a clean build on my system and a lot of crates currently have syn 1.x and 2.x in the dependency tree. Because of syn 2.x being blocked on a 1.0.0 release of derive_more (which seems blocked for other reasons) I suggested removing derive_more and replacing with the handrolled implementations oin #55 .

Initially I started doing this in src/frame.rs manually until I realised just how many impls there were to replace. After that I took the approach of using cargo expand and cleaning up the names and impled code to look less generated.

One thing changed - all the display impls previously had #[inline] as a hint I didn't persist that but I could add it in just to ensure performance doesn't change.

Ignoring dev-dependencies for a user of this project the dependency tree would look like:

ruzstd v0.6.0 (/home/daniel/personal/zstd-rs)
├── byteorder v1.5.0
└── twox-hash v1.6.3
    ├── cfg-if v1.0.0
    └── static_assertions v1.1.0

Whereas before it was:

ruzstd v0.6.0 (/home/daniel/personal/zstd-rs)
├── byteorder v1.5.0
├── derive_more v0.99.17 (proc-macro)
│   ├── proc-macro2 v1.0.82
│   │   └── unicode-ident v1.0.12
│   ├── quote v1.0.36
│   │   └── proc-macro2 v1.0.82 (*)
│   └── syn v1.0.109
│       ├── proc-macro2 v1.0.82 (*)
│       ├── quote v1.0.36 (*)
│       └── unicode-ident v1.0.12
└── twox-hash v1.6.3
    ├── cfg-if v1.0.0
    └── static_assertions v1.1.0

@KillingSpark
Copy link
Owner

Thanks for the effort! I'll review it soon :)

@xd009642
Copy link
Contributor Author

There we go, clippy should now be happy!

@xd009642
Copy link
Contributor Author

And sorry about that fat fingered what was meant to be a simple change, all fixed now and I've ran cargo-hack and clippy locally and it passes as expected

@KillingSpark KillingSpark merged commit 5265c12 into KillingSpark:master May 13, 2024
2 checks passed
@KillingSpark
Copy link
Owner

Thanks a lot! :)

@xd009642
Copy link
Contributor Author

No worries 😁 . Btw any timeline for the next release?

@KillingSpark
Copy link
Owner

I really wanna get my own PR resolved before that, it will increase performance by a few percent. I have a changeset locally that seems to do what I want. If this doesn't work out soon, I'll make a release. The PR I have there is purely internal changes so would be a minor version release, so not a big deal to push it back a bit.

@JelteF
Copy link

JelteF commented May 17, 2024

Author of derive_more here. The 1.0.0-beta.6 release is really stable (it fixes many things compared to than the 0.99.x branch).

@khuey
Copy link

khuey commented May 27, 2024

It would be nice if you could cut a release with this PR in it. We're considering using ruzstd to provide support for zstd-compressed debug info in the backtrace crate, but the dependency on an old version of syn is a blocker to that.

@JelteF
Copy link

JelteF commented May 27, 2024

The 1.0.0-beta.6 release of derive_more depends on the 2.0 version of syn. I believe that should work for you.

@JelteF
Copy link

JelteF commented May 27, 2024

@khuey oh wait I misunderstood. You're asking for a release of zstd-rs not derive_more

@khuey
Copy link

khuey commented May 27, 2024

@khuey oh wait I misunderstood. You're asking for a release of zstd-rs not derive_more

Yeah, that's correct.

I don't think we care whether this crate depends on syn 2 via derive-more 1.0 or whether it doesn't depend on syn at all but we don't want it to depend on syn 1.

@KillingSpark
Copy link
Owner

@khuey If I don't find the time to work further on #58 this week I'll cut a release on the weekend. So either way next week there will be a release without the syn 1.0 dependency

@khuey
Copy link

khuey commented May 28, 2024

Great, thanks!

JelteF added a commit to JelteF/zstd-rs that referenced this pull request Jun 15, 2024
JelteF added a commit to JelteF/zstd-rs that referenced this pull request Jun 15, 2024
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