Add support for no_std #70

Amanieu · 2016-01-12T19:36:57Z

Second attempt at support no_std, this time without splitting the crate into two. Instead, a use_std feature is used, which is enabled by default.

Fixes #13

alexcrichton · 2016-01-12T21:07:11Z

src/lib.rs

@@ -150,10 +190,17 @@
 #![warn(missing_docs)]
 #![cfg_attr(feature = "nightly", feature(panic_handler))]

+#![cfg_attr(all(not(feature = "use_std"), not(test)), no_std)]


I believe that no_std should be usable even during --test, so the not(test) clause shouldn't be necessary here

The tests actually depend on libstd since they make use of .to_string().

In that case you can have an explicit extern crate std in the mod test and use the libstd prelude

I tried it and it doesn't work because of the extern crate core as std; line just below. I think it's fine to just leave the this line as it is.

I mean putting the crate in the module it's needed, not up here. This is a core crate of the Rust ecosystem and is likely to be used as an example for downstream code (e.g. idioms and such), so I'd prefer to have it as idiomatic as possible.

Sorry, I was being stupid with namespaces. I've moved the extern crate std into the tests module.

On Thu, Jan 14, 2016 at 10:21 PM, Alex Crichton notifications@github.com
wrote:

In src/lib.rs
#70 (comment):

@@ -150,10 +190,17 @@
#![warn(missing_docs)]
#![cfg_attr(feature = "nightly", feature(panic_handler))]

+#![cfg_attr(all(not(feature = "use_std"), not(test)), no_std)]

I mean putting the crate in the module it's needed, not up here. This is a
core crate of the Rust ecosystem and is likely to be used as an example for
downstream code (e.g. idioms and such), so I'd prefer to have it as
idiomatic as possible.

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang-nursery/log/pull/70/files#r49794509.

Amanieu · 2016-01-12T21:41:35Z

Updated with given feedback.

sfackler · 2016-01-14T05:13:19Z

.travis.yml

@@ -1,14 +1,15 @@
 language: rust
 sudo: false
 rust:


Can we continue testing against our minimum supported Rust version, which'll be 1.4.0 I believe.

Hm which part requires 1.4.0? Ideally this would continue to compile on 1.0.0 as long as possible, but if it's too hard it may not be worth it

I think the code depends on these feature:

Box::into_raw

Box::from_raw

(*const Trait) as *const Struct (used in one of the doc tests)

The first two can easily be changed into mem::transmute, but I'm not sure about the last one.

Ah ok, would it be possible to leverage mem::transmute for now then? It's fine to just build the library on 1.0.0 instead of running the full test suite which may require more features.

sfackler · 2016-01-14T05:32:23Z

src/lib.rs


-    unsafe {
-        assert_eq!(libc::atexit(shutdown), 0);
+/// Sets the global logger from a raw pointer.


We should probably note here and in shutdown_logger that they should only be used in a no_std setup.

Note added, shutdown_logger already has a note saying that it shouldn't be used with set_logger.

alexcrichton · 2016-01-14T07:28:17Z

src/lib.rs

+/// This function should not be called when the global logger was registered
+/// using `set_logger`, since in that case the logger will automatically be shut
+/// down when the program exits
+pub fn shutdown_logger() -> Result<*const Log, ShutdownLoggerError> {


This and set_logger_raw may want to be paired up in terms of naming or organization to set them apart from set_logger a bit more (and to make it clear that shutdown_logger isn't necessary most of the time)

So what exactly are you suggesting? shutdown_logger_raw? Or something like log::raw::{set_logger, shutdown_logger}?

I think a module perhaps, but it seems kinda arbitrary. Including raw in the name seems prudent though.

alexcrichton · 2016-01-14T23:36:22Z

@sfackler I'm happy with this modulo a bit of bikeshedding, and I'll leave it up to you to merge

alexcrichton · 2016-01-14T23:36:41Z

Also thanks @Amanieu, I'm excited to see more libs start picking up the no_std standard!

Amanieu · 2016-01-14T23:59:04Z

Updated again, it should work on 1.0.0 now. I also renamed shutdown_logger to shutdown_logger_raw.

sfackler · 2016-01-16T04:33:38Z

src/lib.rs

+
+// The Error trait is not available in libcore
+#[cfg(feature = "use_std")]
+impl error::Error for ShutdownLoggerError {


@alexcrichton there's nothing blocking us from adding Error to libcore right?

Doesn't affect this PR of course.

Yeah I think it should be easy enough to add

This comment explains why Error is in libstd: https://github.com/rust-lang/rust/blob/master/src/libstd/error.rs#L39

Oh right, forgot about that.

sfackler · 2016-01-16T04:34:58Z

Looks good to me, thanks!

Add support for no_std

sfackler · 2016-01-16T04:35:22Z

I'm going to take a pass over the docs to make sure everything looks right and then push a release.

sfackler · 2016-01-16T05:07:38Z

Made a couple minor tweaks: #71

Closes rust-lang#70 Fixes rust-lang#69

Amanieu force-pushed the no_std2 branch from 316ffb7 to 2ad3e3d Compare January 12, 2016 19:40

alexcrichton reviewed Jan 12, 2016
View reviewed changes

Amanieu force-pushed the no_std2 branch 2 times, most recently from 99e066f to 6c8e548 Compare January 12, 2016 21:37

Amanieu mentioned this pull request Jan 12, 2016

Split crate into log and log_core #68

Closed

Amanieu force-pushed the no_std2 branch from 6c8e548 to 0178421 Compare January 12, 2016 21:39

Amanieu force-pushed the no_std2 branch from 0178421 to 83b1deb Compare January 14, 2016 05:01

sfackler reviewed Jan 14, 2016
View reviewed changes

Amanieu force-pushed the no_std2 branch 2 times, most recently from 3c3f89d to de132b7 Compare January 14, 2016 05:24

sfackler reviewed Jan 14, 2016
View reviewed changes

Amanieu force-pushed the no_std2 branch from de132b7 to 8de1238 Compare January 14, 2016 05:45

alexcrichton reviewed Jan 14, 2016
View reviewed changes

Amanieu force-pushed the no_std2 branch from 8de1238 to 831947f Compare January 14, 2016 22:42

Amanieu force-pushed the no_std2 branch from 831947f to 00960a8 Compare January 14, 2016 23:51

Add support for no_std

2644916

Amanieu force-pushed the no_std2 branch from 00960a8 to 2644916 Compare January 14, 2016 23:55

sfackler reviewed Jan 16, 2016
View reviewed changes

sfackler added a commit that referenced this pull request Jan 16, 2016

Merge pull request #70 from Amanieu/no_std2

cb09c6e

Add support for no_std

sfackler merged commit cb09c6e into rust-lang:master Jan 16, 2016

EFanZh pushed a commit to EFanZh/log that referenced this pull request Jul 23, 2023

Merge pull request rust-lang#147 from passcod/cli-overrides

f0e7fa0

Closes rust-lang#70 Fixes rust-lang#69

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for no_std #70

Add support for no_std #70

Amanieu commented Jan 12, 2016

alexcrichton Jan 12, 2016

Amanieu Jan 12, 2016

alexcrichton Jan 14, 2016

Amanieu Jan 14, 2016

alexcrichton Jan 14, 2016

Amanieu Jan 14, 2016

Amanieu commented Jan 12, 2016

sfackler Jan 14, 2016

Amanieu Jan 14, 2016

alexcrichton Jan 14, 2016

Amanieu Jan 14, 2016

alexcrichton Jan 14, 2016

sfackler Jan 14, 2016

Amanieu Jan 14, 2016

alexcrichton Jan 14, 2016

Amanieu Jan 14, 2016

alexcrichton Jan 14, 2016

alexcrichton commented Jan 14, 2016

alexcrichton commented Jan 14, 2016

Amanieu commented Jan 14, 2016

sfackler Jan 16, 2016

alexcrichton Jan 16, 2016

Amanieu Jan 16, 2016

alexcrichton Jan 16, 2016

sfackler commented Jan 16, 2016

sfackler commented Jan 16, 2016

sfackler commented Jan 16, 2016

Add support for no_std #70

Add support for no_std #70

Conversation

Amanieu commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amanieu commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jan 14, 2016

alexcrichton commented Jan 14, 2016

Amanieu commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler commented Jan 16, 2016

sfackler commented Jan 16, 2016

sfackler commented Jan 16, 2016