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 serde rc feature #31317

Open
rillian opened this issue Jun 26, 2023 · 2 comments
Open

Remove serde rc feature #31317

rillian opened this issue Jun 26, 2023 · 2 comments

Comments

@rillian
Copy link

rillian commented Jun 26, 2023

Description

One of our rust dependencies is enabling the rc feature of the rust serde crate, which isn't recommended since it's easy to misuse. I'd like to track down who needs it and why, in case we can remove it, especially since we're moving to a static build description.

Steps to Reproduce

  1. cd build/rust
  2. cargo metadata | jq . | less
  3. search for "name": "serde"

Actual result:

features array includes rc

Expected result:

Reproduces how often:

Brave version (brave://version info)

True as of 1.55.15.

@rillian
Copy link
Author

rillian commented Jun 28, 2023

This is coming from adblock-rust

@antonok-edm antonok-edm self-assigned this Jul 13, 2023
@antonok-edm
Copy link
Collaborator

@rillian the code predates me, but it's from here - NetworkFilterList uses a map from tokens to filters, where each filter can be pointed to by potentially multiple tokens.

As you note, it's easy to misuse, and indeed it looks like it was misused 😅. Serializing and deserializing a DAT with filters pointed to by multiple tokens would duplicate the tokens.

We no longer use the DAT format in brave-core for several reasons, one of which is that it generally ends up being larger than the plaintext lists (maybe this misuse has something to do with that!). I don't plan on maintaining the DAT handling code further; removing it altogether would be a simple solution. Unfortunately, we still need it on iOS until #36035 is completed and enough clients have updated to a version that includes that change.

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

No branches or pull requests

3 participants