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

serde_derive brings binary blob and breaks builds #1333

Closed
pinkforest opened this issue Aug 19, 2023 · 6 comments
Closed

serde_derive brings binary blob and breaks builds #1333

pinkforest opened this issue Aug 19, 2023 · 6 comments

Comments

@pinkforest
Copy link

pinkforest commented Aug 19, 2023

serde_derive has chosen interesting approach

I've sent PR to make the binary blob opt-in approach so the top-level binary chooses the desired build / trust model instead

This breaks Nix builds etc so I would recommend restricting the SemVer scope on serde_derive for dev-dep and serde for dep

It was labeled as "Experimental" to embed the binary blob but it went straight to prod :(

[dependencies]
serde = { version = "1.0.99, <= 1.0.171", optional = true, default-features = false, features = ["serde"] }

The above should also restrict the serde_derive - time and rust-analyzer have already restricted the SemVer range otherwise

EDIT: fyi Yeah looks like serde has chosen the opt-out path only so the only way to not get the blobs is to restrict.

@newpavlov
Copy link
Member

I support pinning, but current master branch tracks future v0.9 release of rand, so we would need to do a backport.

@pinkforest
Copy link
Author

Can we add a origin branch 0.8 with:

git rebase --onto 0.8.5 origin/master

I have a PR ready to go to it as well - I will send another for master

@pinkforest
Copy link
Author

pinkforest commented Aug 19, 2023

I have a PR up against master on

I will send another if someone creates the 0.8 brach as above

Also looks like serde is only going for opt-out blob approach regrettably..

Cc/ @vks @dhardy

@dhardy
Copy link
Member

dhardy commented Aug 20, 2023

Instead of jumping on the current hot potato, I suggest we hold off on taking any action. It won't affect users anyway since we don't plan on making any releases soon.

Edit: sorry for the knee-jerk reaction. I saw that this already blew up on Reddit. But pinning old versions of Serde in libraries does not appear a good solution:

  • Many libraries are required to publish new versions just to white/blacklist versions of an (optional) dependency, and to publish new versions again if future releases of serde_derive revert to the traditional proc-macro system.
  • It prevents use of new versions of serde_derive patching other issues by users who do not care about the pre-compiled binary issue.
  • It may completely break some builds (if other crates require newer versions of serde_derive).

My suggestions:

  1. Wait. Most likely some other solution will emerge in the near future. Possibly Mr Tolnay will revert to non-precompiled binaries.
  2. Develop a better model for pre-compiled proc-macros — but this is not a quick solution and not justification for the current change.
  3. Fork Serde (or build a complete new alternative).
  4. Simply remove serde as a dep. I don't know how it is used by Rand users, but it is not a key dependency of Rand (many RNGs do not even support serialisation, deliberately). This would have to be a breaking release however.

@vks
Copy link
Collaborator

vks commented Aug 20, 2023

Can't crates depending on an RNG and using the serde1 feature just specify the version they prefer? This seems better than forcing some decision on all users.

@pinkforest
Copy link
Author

This has been resolved in upstream 🎉

Also see time: time-rs/time#614

@pinkforest pinkforest closed this as not planned Won't fix, can't repro, duplicate, stale Aug 21, 2023
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

No branches or pull requests

4 participants