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

Allow bitflags v1 and v2 to coexist #877

Open
Urhengulas opened this issue Oct 18, 2024 · 3 comments · May be fixed by #894
Open

Allow bitflags v1 and v2 to coexist #877

Urhengulas opened this issue Oct 18, 2024 · 3 comments · May be fixed by #894
Labels
good first issue Good for newcomers

Comments

@Urhengulas
Copy link
Member

There is a defmt::bitflags! macro which depends on the bitflags::bitflags! macro from the bitflags v1 crate. Since it's introduction there was a bitflags v2, but unfortunately we cannot just bump the dependency, since the defmt::bitflags! macro leaks implementation details of the bitflags::bitflags! macro and therefore the dependency update could break the code of defmt users.

Therefore we want to add two new cargo features to defmt: bitflags1 and bitflags2. They each gate a defmt::bitflags macro which depends on bitflags v1 and bitflags v2 respectively. To not break any existing code, the bitflags1 feature should be enabled by default. A compile_error when both are enabled sounds useful as well.

Note that there is an existing PR to update to bitflags v2: #746.

@Urhengulas Urhengulas changed the title Allow bitflags version one and two to coexist Allow bitflags v1 and v2 to coexist Oct 18, 2024
@Urhengulas Urhengulas added the good first issue Good for newcomers label Oct 18, 2024
@jonathanpallant
Copy link
Contributor

jonathanpallant commented Nov 27, 2024

The important note here is co-exist. Any support for bitflags v2 has to use a new name for the macro, and we have to support the old one at the same time.

Imagine Application X depends on Library A and Library B. Library A wants to use the existing bitflags API but Library B is newer and wants to use the newer bitflags v2 API (that we haven't added yet, but that we might add). Both crates need to be compiled using the exact same set of cargo features, because that's how cargo works unfortunately.

@Urhengulas Urhengulas linked a pull request Nov 28, 2024 that will close this issue
@Urhengulas
Copy link
Member Author

True. So I propose the following:

  • hide defmt::bitflags behind a feature but enable it by default
  • add bitflags v2 as defmt::bitflags2! behind a feature, not enabled by default

@jonathanpallant do you see any problems with that approach?

cc @manushT

@jonathanpallant
Copy link
Contributor

Do we have any existing features and is anyone out there likely to be using defmt with default-features = false? Because that proposal would break them.

The other option is:

  1. Leave defmt::bitflags as it is
  2. Add a new feature bitflagsv2 which provides defmt::bitflagsv2 using an optional dependency.

It's still not unclear to be if anyone needs us to upgrade to bitflags v2. It's an improvement, but I don't think bitflags v1 is broken for anyone right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants