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

Support no_std #52

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

Support no_std #52

wants to merge 6 commits into from

Conversation

TTWNO
Copy link

@TTWNO TTWNO commented Jun 5, 2024

I'm writing a library that needs to be tested inside no_std environments.
Since tracing, and tracing-subscriber do not inherently require std, 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 switched std::env::var_os with core::option_env, which resolves an optional environment variable at compile time. I'm fine feature gating the std::env::var_os call behind an std feature if that is more acceptable for you.

Some things to consider:

  • You may want to enable std_instead_of_alloc to identify types up front that can be easily used without std
  • You may want an std flag to enable more features available via std that are not required for the base crate to work.

Thanks for all your work; I really enjoy this crate!

—Tait

std::env -> core::option_env
@d-e-s-o
Copy link
Owner

d-e-s-o commented Jun 6, 2024

Thanks for the pull request. For my understanding, what would be the motivation for running in a no_std environment?
I get why a "production" crate wants to be no_std compatible, but this is a development crate, specifically designed to be used as a dev-dependency. I fail to see why one would want to have the associated constraints when running tests.

@TTWNO
Copy link
Author

TTWNO commented Jun 6, 2024

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.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Jun 7, 2024

So you'd compile for the target, which doesn't have a Rust std environment, set the environment variables during compilation, copy the resulting binaries over, and then run the test? Is that the envisioned workflow?

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?

@TTWNO
Copy link
Author

TTWNO commented Jun 10, 2024

So you'd compile for the target, which doesn't have a Rust std environment, set the environment variables during compilation, copy the resulting binaries over, and then run the test? Is that the envisioned workflow?

Yes.

Clearly it is not in any state to be merged.

Just missing a few clippy/fmt issues. Trivial fixup in the next few hours and it should be ready to go! :)

@TTWNO
Copy link
Author

TTWNO commented Jun 10, 2024

Looks like the clippy lint is failing on your main. Not sure exactly when this is from, but this is not due to a change in my PR.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Jun 10, 2024

Looks like the clippy lint is failing on your main. Not sure exactly when this is from, but this is not due to a change in my PR.

Probably due to a new Rust release. Will fix it up.

Copy link
Owner

@d-e-s-o d-e-s-o left a 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?

Comment on lines 202 to 209
match ::std::env::var_os("RUST_LOG_SPAN_EVENTS") {
match ::core::option_env!("RUST_LOG_SPAN_EVENTS") {
Copy link
Owner

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?

Comment on lines -204 to -205
value.make_ascii_lowercase();
let value = value.to_str().expect("test-log: RUST_LOG_SPAN_EVENTS must be valid UTF-8");
Copy link
Owner

Choose a reason for hiding this comment

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

Similar comment as above.

@TTWNO
Copy link
Author

TTWNO commented Jun 14, 2024

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?

tracing_subscriber has an std feature that you disable by default already, no problems there!

env_logger sort of fundamentally requires std; I have now made log depend on the std feature.

Since I only use trace this is no issue for me. Test added for trace

@d-e-s-o
Copy link
Owner

d-e-s-o commented Jun 23, 2024

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?

tracing_subscriber has an std feature that you disable by default already, no problems there!

It works because env-filter enables std. Hence, there is an std environment available anyway. Then, what's the point of making this crate support a no_std environment if other dependencies are free to use std?

@dvdplm
Copy link

dvdplm commented Dec 19, 2024

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 #![cfg_attr(not(test), no_std)] instead may or may not be possible and poses at the very least a papercut to adoption. Is it truly necessary for test-log to enable the env-filter feature or can it work without it?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Dec 21, 2024

It is what is evaluating RUST_LOG, so it seems pretty crucial to me.

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