-
Notifications
You must be signed in to change notification settings - Fork 35
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
Support no_std
#52
base: main
Are you sure you want to change the base?
Support no_std
#52
Conversation
std::env -> core::option_env
Thanks for the pull request. For my understanding, what would be the motivation for running in a |
I understand this is meant as a dev-dep. The tests in my case need to be able to be run on the target machine, not just on any machine. |
So you'd compile for the target, which doesn't have a Rust I guess that seems like something we could conceivably support. And what is the intention of this pull request? Clearly it is not in any state to be merged. Are you asking for input? Do you want me to finish it? |
Yes.
Just missing a few clippy/fmt issues. Trivial fixup in the next few hours and it should be ready to go! :) |
Looks like the clippy lint is failing on your |
Probably due to a new Rust release. Will fix it up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now this crate is no_std
compatible but we just use tracing-subscriber
and env_logger
as they are (i.e., with std
enabled unconditionally). How is that going to satisfy your workflow? Can you please add a no_std
test that checks no_std
support end-to-end?
Also, my understanding is that methods such as https://docs.rs/tracing-subscriber/0.3.18/tracing_subscriber/fmt/struct.SubscriberBuilder.html#method.with_env_filter won't even be present. Yet, using environment variables is basically the way this crate is configured. Do you intend to work with tracing-subscriber
and env_logger
to use option_env
for no_std
contexts?
macros/src/lib.rs
Outdated
match ::std::env::var_os("RUST_LOG_SPAN_EVENTS") { | ||
match ::core::option_env!("RUST_LOG_SPAN_EVENTS") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should preserve the runtime behavior, at least for std
contexts. Can you introduce an std
feature and make this conditional?
value.make_ascii_lowercase(); | ||
let value = value.to_str().expect("test-log: RUST_LOG_SPAN_EVENTS must be valid UTF-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as above.
Since I only use |
It works because |
It's not uncommon that no_std crates want their test suites to run in no_std as well. Convincing maintainers to switch to using |
It is what is evaluating |
I'm writing a library that needs to be tested inside
no_std
environments.Since
tracing
, andtracing-subscriber
do not inherently requirestd
, it would be convenient to be able to use this crate to do so.I've made some trivial changes, like importing
alloc::{string::String, vec::Vec}
, but the more important change is that I've switchedstd::env::var_os
withcore::option_env
, which resolves an optional environment variable at compile time. I'm fine feature gating thestd::env::var_os
call behind anstd
feature if that is more acceptable for you.Some things to consider:
std_instead_of_alloc
to identify types up front that can be easily used withoutstd
std
flag to enable more features available viastd
that are not required for the base crate to work.Thanks for all your work; I really enjoy this crate!
—Tait