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

fix: enable alloc feature on hex crate #3000

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

yanCode
Copy link
Contributor

@yanCode yanCode commented Sep 17, 2024

Description

I detected this issue while i was trying to run unit test under an individual crate bytes-hex.
I mean to execute cargo build or cargo test right under ./crates/bytes-hex. well, as bytes-hex is a crate, it's supposed to compile sucessfully & do unit tests independently. but it cannot even compile.
with an error like:

296:8
    |
296 | pub fn decode<T: AsRef<[u8]>>(data: T) -> Result<Vec<u8>, FromHexError> {
    |        ^^^^^^
note: the item is gated behind the `alloc` feature

but I also realized the unit tests working well in github Actions. then i forked the code and test a few more scenarios, for examples, if you modify the Cargo.toml in the root directory, changing
members = ["crates/*"]
into
members = ["crates/bytes-hex"]
then the github actions will fail with the exact same error, here is the github action log from my forked repo FYI.
however, as long as i include some crates which including bytes-hex as dependency, like
members = ["crates/model", "crates/bytes-hex"]
It works successfully as the default which including all crates. I mean members = ["crates/*"]

To this point, i realised that these crates like model much have smuggled the feature into bytes-hex somehow.
I did a lot of research, in the official doc of rust, i learned that is a concept Feature unification. In short, in a workplace if multi crates/dependencies refer to the same crate, as long as these refers can be resolved to the same version, it will make an union of features from each reference. and the union of features will be available globally among the whole workplace.

for example, web3 crate is commonly refered as a dependency in many crates, including model. if you step into the dependency of web3 crate . you can see hex is included with default features, that is it comes with alloc feature. because of this the feature alloc is propogating to bytes-hex. that's why it works if putting bytes-hex with other crates, it works fine.
(N.B: hex is a commonly used crate with default feature in many dependencies from the crates in the workplace, you can using cargo tree --all-features to check.)

Changes

As long as i can confirm the feature alloc of hex crate will be introduced by other dependencies into the workplace. so just update Cargo.toml inside bytes-hex. this can ensure tasks like build, unit test work inside bytes-hex and in the workplace level. while introducing very less (or even no) impacts to the whole workplace.

#How to test
now it can be tested freely either inside bytes-hex or in the workplace. in .devcontainer or in github actions.

Why open this PR

I open this PR because this is the first time I learnt how feature unification in rust exactly works although it claimed a few hours of mine with which I planed to learn some CoW knowledge.

@yanCode yanCode requested a review from a team as a code owner September 17, 2024 12:54
Copy link

github-actions bot commented Sep 17, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@yanCode
Copy link
Contributor Author

yanCode commented Sep 17, 2024

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Sep 17, 2024
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and the very detailed PR description. 👏

@MartinquaXD MartinquaXD enabled auto-merge (squash) September 19, 2024 06:21
@MartinquaXD MartinquaXD merged commit 893613d into cowprotocol:main Sep 19, 2024
10 of 11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
@yanCode yanCode deleted the fix/feature-of-hex branch September 19, 2024 08:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants