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

Move hashbrown behind no_std feature #168

Merged
merged 3 commits into from
Aug 14, 2023
Merged

Conversation

grovesNL
Copy link
Contributor

@grovesNL grovesNL commented Aug 9, 2023

Follow-up to #160 to gate hashbrown behind no_std by default

cc @notgull please let me know if this is problematic for some reason

@grovesNL
Copy link
Contributor Author

grovesNL commented Aug 9, 2023

Looks like this breaks no-default-features, I'll try to workaround that

@grovesNL
Copy link
Contributor Author

grovesNL commented Aug 9, 2023

Unfortunately I don't think we have a great way to do this with the current features, because no_std and std features would ideally be mutually exclusive but both features could be enabled or disabled at the same time. Maybe we could revisit if we ever change how these features are setup.

(I didn't notice that std was also a feature initially)

Related to rust-lang/cargo#2980

@grovesNL grovesNL closed this Aug 9, 2023
@grovesNL grovesNL deleted the nostd-hash branch August 9, 2023 13:15
@grovesNL grovesNL restored the nostd-hash branch August 9, 2023 13:52
@grovesNL grovesNL reopened this Aug 9, 2023
@grovesNL
Copy link
Contributor Author

grovesNL commented Aug 9, 2023

Thinking about this some more, building with no default features doesn't make sense because either no_std or std must be used. We should replace this with a CI step to check no_std. I'll update this PR to add a compile-time error if neither is specified.

jackpot51
jackpot51 previously approved these changes Aug 9, 2023
@ids1024
Copy link
Member

ids1024 commented Aug 9, 2023

Having both an std and a no_std feature isn't ideal. It should be possible to use #cfg(not(feature = "std")), but enabling dependencies and features of dependencies when a feature is disabled won't really work...

So I guess requiring either std or no_std makes sense, though hopefully cargo can get some better solution to this sort of thing some day.

@grovesNL
Copy link
Contributor Author

grovesNL commented Aug 9, 2023

@ids1024 agreed, it would be great if we didn't need to have both. In case it isn't clear, both features already exist in cosmic-text, so this PR is just attempting to move hashbrown behind the no_std feature.

@jackpot51 jackpot51 merged commit 618896f into pop-os:main Aug 14, 2023
2 checks passed
@grovesNL grovesNL deleted the nostd-hash branch August 14, 2023 12:53
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.

3 participants