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 string.c to be disabled when it conflicts with other libs #15

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

liebman
Copy link
Contributor

@liebman liebman commented Sep 5, 2024

There are other crates that are based on C libs and define various functions like strchr. This PR adds a feature that when enabled does not compile string.c and expects some external source to provide the functions within.

I ran into this issue with the current (unreleased) esp-wifi that when combined with littlefs2 fails to link because strchr is multiply defined.

When combining crates that do this one can disable string.c and if needed add this to fill in any functions that are still missing:

tinyrlibc = { version = "0.4.0", features = ["strspn", "strcspn", "strlen"] }

@robin-nitrokey
Copy link
Member

Having features that disable code is somewhat counter-intuitive for me. But I’m not sure if a positive feature that enables strings.c would really be better here. Do you have a preference?

@liebman
Copy link
Contributor Author

liebman commented Nov 11, 2024

I have no preference - as long as there is control as to where these functions come from.

@sosthene-nitrokey
Copy link
Contributor

From the Cargo book

A consequence of this is that features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features. A feature should not introduce a SemVer-incompatible change.

Another solution would be to expect the user to always bring their own implementation. This is only matters for embedded so it's not shocking.

@liebman
Copy link
Contributor Author

liebman commented Nov 12, 2024

Another solution would be to expect the user to always bring their own implementation. This is only matters for embedded so it's not shocking.

Thats fine too. One can always use

tinyrlibc = { version = "0.4.0", features = ["strspn", "strcspn", "strlen"] }

to bridge the gap.

@robin-nitrokey
Copy link
Member

I don’t see strspn and strcspn in tinyrlibc v0.4.0. Am I missing something?

@liebman
Copy link
Contributor Author

liebman commented Nov 19, 2024

Your correct :-(

@robin-nitrokey
Copy link
Member

In that case I would vote for keeping the functions in littlefs2-sys but use the feature flag to enable them (instead of disabling them if the feature is set) – mabye just call it string-funcs?

@liebman
Copy link
Contributor Author

liebman commented Nov 19, 2024

That works for me.

@sosthene-nitrokey
Copy link
Contributor

I'll open an issue on tinyrlibc, honestly being able to get rid of theses implementation seems good (and using a rust implementation instead too).

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