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

Implement Support for no_std #341

Merged
merged 15 commits into from
Nov 22, 2019
Merged

Implement Support for no_std #341

merged 15 commits into from
Nov 22, 2019

Conversation

CryZe
Copy link
Contributor

@CryZe CryZe commented Sep 7, 2019

This adds a new std feature to chrono that is enabled by default. By deactivating this feature via default-features = false you can now use chrono in applications that don't use the standard library. The serde feature is supported as well.

Resolves #336

This adds a new `std` feature to chrono that is enabled by default. By
deactivating this feature via `default-features = false` you can now use
chrono in applications that don't use the standard library. The `serde`
feature is supported as well.

Resolves chronotope#336
@CryZe
Copy link
Contributor Author

CryZe commented Sep 7, 2019

Well looks like I definitely need help with that CI script.

@quodlibetor
Copy link
Contributor

Thank you! I will look at the CI script.

@quodlibetor quodlibetor self-assigned this Sep 7, 2019
Copy link
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

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

I've got a couple nits that I'd be happy to take care of if you don't feel like it, otherwise this seems great!

ci/travis.sh Outdated Show resolved Hide resolved
use std::time::{SystemTime, UNIX_EPOCH};
use oldtime::Duration as OldDuration;

#[cfg(not(any(feature = "std", test)))]
use alloc::string::{String, ToString};
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it might be worth sticking this (and what seems like the two functions that return strings) behind a separate alloc feature flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an alloc feature now. I'm kind of sad how much is missing without it though. And it's mostly only because the timezone offset is stored as a String in the DelayedFormat.

mod core_only {
/// Core only
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord)]
pub struct Box<T: ?Sized>(core::marker::PhantomData<T>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This thing is kind of hacky, but it allows at least partially supporting some of the format module stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain how/if this actually works/what the runtime experience would be? Or is there some explanation of this pattern somewhere?

Alternatively, would you mind adding some small code to the core test file that demonstrates what this ends up doing?

Copy link
Contributor Author

@CryZe CryZe Sep 13, 2019

Choose a reason for hiding this comment

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

It's only so the Item enum actually compiles as there's Box<str> in there. I'm not sure if we even need this enum when alloc isn't compiled in. I honestly don't know what even works in that module at all if DelayedFormat isn't compiled in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the timezone for the DelayedFormat really need to be a String or could it be turned into a &'static str? (I'm not sure if there ever are any truly dynamic timezones that would need String)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, what is OwnedLiteral and OwnedSpace even used for? I don't see them being used anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I ended up putting these variants behind the same feature gate, I'm not sure that it will have any effect on usability. Potentially if people were compiling their own format::Item lists it could have been a thing?

@CryZe
Copy link
Contributor Author

CryZe commented Sep 9, 2019

Also with alloc this is now slightly a breaking change as the serde feature is now called serde-1, because you can't have feature dependencies on optional crates themselves (for whatever reason).

Although it requires a semver incompatible version bump anyway, as adding new features such as std and alloc would be incompatible with someone using chrono with default-features = false.

To ensure that we don't accidentaly not verify that chrono compiles for core.
@quodlibetor
Copy link
Contributor

Also.. this is now slightly a breaking change

Hm that's a good point, I'll need to think about that. Does SemVer apply to compilation flags? I've taken things out of the --no-default-features install without bumping the patch version before and I'm not sure that I've gotten bug reports about it. This is a much more dramatic change than before, though, so maybe not.

as the serde feature is now called serde-1, because you can't have feature dependencies on optional crates themselves.

Yeah that's true, but you can rely on optional dependencies as though they're features, so I think that it's fine to just use serde, and it seems to work for me.

@CryZe
Copy link
Contributor Author

CryZe commented Sep 13, 2019

I believe if you activate the serde feature without the alloc feature now, you get weird compilation errors. I guess if it's properly documented that you need to also activate alloc then, it might not be a problem.

Or you could even do something like:

#[cfg(all(feature = "serde", not(feature = "alloc"))]
compile_error!("For the serde feature you also need the alloc feature.");

(this is not the full cfg, as std and test are also valid)

@quodlibetor
Copy link
Contributor

Yeah that's a good point, I didn't quite realize that it was possible for people to want to be able to use serde without std, but in retrospect it makes sense.

@quodlibetor
Copy link
Contributor

Just to be clear, you mean that serde requires the alloc feature, so doing --no-default-features --features serde that will start failing, but I would expect just doing --features serde would work. And that's what appears to be the case, but I might be missing something.

@CryZe
Copy link
Contributor Author

CryZe commented Sep 13, 2019

I'd be surprised if it works, but I guess it might? I'll check tomorrow.

Slightly easier to reason about the code via some code movement, printing some
banners to make it more obvious when cargo is being run since it is run so many
times.
Part of this means that we don't actually need to allocate to write to Serde
any more, which is a win in its own right.
@quodlibetor
Copy link
Contributor

I just pushed a change that should remove all intermediary allocation even when the serde feature is enabled. I didn't look into how that plays with the core::box placeholder.

I tried to simplify the features, as well, let me know how it feels to you to use this way.

@CryZe
Copy link
Contributor Author

CryZe commented Sep 20, 2019

That does not seem to build if you build with serde and no alloc:

   Compiling chrono v0.4.9 (https://github.com/CryZe/chrono?branch=no-std#e5bbc94c)
error[E0432]: unresolved import `alloc`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:1761:13
     |
1761 |         use alloc::format;
     |             ^^^^^
     |             |
     |             unresolved import
     |             help: a similar path exists: `core::alloc`

error[E0432]: unresolved import `alloc`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:1908:13
     |
1908 |         use alloc::format;
     |             ^^^^^
     |             |
     |             unresolved import
     |             help: a similar path exists: `core::alloc`

error[E0432]: unresolved import `alloc`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:2055:13
     |
2055 |         use alloc::format;
     |             ^^^^^
     |             |
     |             unresolved import
     |             help: a similar path exists: `core::alloc`

error: cannot find macro `format` in this scope
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/time.rs:1448:51
     |
1448 |             value.parse().map_err(|err| E::custom(format!("{}", err)))
     |                                                   ^^^^^^

error: cannot find macro `format` in this scope
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:1711:51
     |
1711 |             value.parse().map_err(|err| E::custom(format!("{}", err)))
     |                                                   ^^^^^^

error: cannot determine resolution for the macro `format`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:1857:46
     |
1857 |                     .ok_or_else(|| E::custom(format!("value is not a legal timestamp: {}", value)))
     |                                              ^^^^^^
     |
     = note: import resolution is stuck, try simplifying macro imports

error: cannot determine resolution for the macro `format`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:1865:46
     |
1865 |                     .ok_or_else(|| E::custom(format!("value is not a legal timestamp: {}", value)))
     |                                              ^^^^^^
     |
     = note: import resolution is stuck, try simplifying macro imports

error: cannot determine resolution for the macro `format`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:2004:46
     |
2004 |                     .ok_or_else(|| E::custom(format!("value is not a legal timestamp: {}", value)))
     |                                              ^^^^^^
     |
     = note: import resolution is stuck, try simplifying macro imports

error: cannot determine resolution for the macro `format`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:2012:46
     |
2012 |                     .ok_or_else(|| E::custom(format!("value is not a legal timestamp: {}", value)))
     |                                              ^^^^^^
     |
     = note: import resolution is stuck, try simplifying macro imports

error: cannot determine resolution for the macro `format`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:2150:46
     |
2150 |                     .ok_or_else(|| E::custom(format!("value is not a legal timestamp: {}", value)))
     |                                              ^^^^^^
     |
     = note: import resolution is stuck, try simplifying macro imports

error: cannot determine resolution for the macro `format`
    --> /home/user/.cargo/git/checkouts/chrono-5169c63406b50cd5/e5bbc94/src/naive/datetime.rs:2157:46
     |
2157 |                     .ok_or_else(|| E::custom(format!("value is not a legal timestamp: {}", value)))
     |                                              ^^^^^^
     |
     = note: import resolution is stuck, try simplifying macro imports

error: aborting due to 11 previous errors

@quodlibetor
Copy link
Contributor

Oh, I thought i had fixed that correctly, I'll get it working.

@quodlibetor
Copy link
Contributor

This seems like it's working on stable, now. The failures had to do with using core in 1.13, which I'm not super interested in debugging unless someone actively wants it.

@CryZe
Copy link
Contributor Author

CryZe commented Sep 23, 2019

Yeah it seems to be working now across all the different combinations :)

@quodlibetor
Copy link
Contributor

WooHoo!

Okay I'd like to see what we can do about the stub core module that you created, ideally we can make it unnecessary. I doubt I'll have any time to look at it before friday, if you're interested.

@CryZe
Copy link
Contributor Author

CryZe commented Oct 30, 2019

Do you still want me to check it? I don't really understand the formatting module enough, but I could take a closer look and try to understand it if you don't have time to look into it yourself. I'd like to get this merged (and released) fairly soon, as the other crates that are blocking me almost all have released new versions and the few remaining are intending to do so within the next weeks.

This means that a few more features of formatting items don't compile in
non-alloc environments, but they wouldn't have worked correctly anyway.
@quodlibetor
Copy link
Contributor

I am happy with this as is now, we can add and improve things as they come up. Thanks for this!

@quodlibetor quodlibetor merged commit d9929a6 into chronotope:master Nov 22, 2019
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.

no_std support
2 participants