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.

Sign up for GitHub

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

Jump to bottom

Allow tracing-serde to work on no_std. #960

Merged
merged 5 commits into from
Oct 1, 2020
Merged

Allow tracing-serde to work on no_std. #960

merged 5 commits into from
Oct 1, 2020

Conversation

Hoverbear
Copy link
Contributor

Allows tracing-serde to work on devices without std.

  • Enable #[no_std] in tracing-serde when cfg(not(feature = std))
  • No longer have unnecessary dependencies on std in serde/tracing.

No behavior differences or actual code changes other than allowing for core/alloc fallbacks.

@Hoverbear Hoverbear requested review from hawkw and a team as code owners August 29, 2020 13:36
@Hoverbear
fmt
8809517 
Signed-off-by: Ana Hobden <[email protected]>
@hawkw
Copy link
Member

hawkw commented Aug 30, 2020

I was concerned that this might be a breaking change for anyone who depended on tracing-serde with default-features = false, but I realize we didn't previously have any feature flags for that crate...I think this is not breaking.

Do you happen to know if it's allowed to set default-features = false for a crate with no feature flags? If it is, anyone who has done that would no longer have serde's std feature enabled, unless enabled elsewhere...

@Hoverbear
Copy link
Contributor Author

It seems to be allowed.

image

@hawkw
Copy link
Member

hawkw commented Aug 31, 2020

It seems to be allowed.

image

Hmm. Is it possible that this could break anything? I'm not sure.

Presumably, if you're using tracing-serde, you're also depending on serde (or serde-json or something), and those dependencies would enable serde's default features unless you've explicitly disabled them, so disabling the default features on tracing-serde's serde dependency wouldn't break APIs you expect to be there...right?

Sorry for drilling so deep into this, I'd just really like to work out whether we can ship this change in a patch or if it has to be a breaking release. Normally we'd definitely consider feature flagging something that wasn't previously feature flagged a breaking change, but in this case, I'm not sure...

@Hoverbear
Copy link
Contributor Author

I absolutely think it's fine to wait for a breaking compatible release for this. I don't want to break anyone's production code just so I don't need a Git dependency for a few weeks. :)

Thanks so much Eliza.

jyn514
jyn514 reviewed Sep 15, 2020
View reviewed changes
tracing-serde/src/lib.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Sep 29, 2020

Once #998 merge, everything on the master branch will be versioned for 0.2.x, so we can actually land this soon for tracing-serde 0.2

hawkw
hawkw reviewed Sep 30, 2020
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Mind rebasing this onto the latest master and adding the feature flag to the docs? Once that's taken care of, this should be good to merge!

tracing-serde/Cargo.toml Show resolved Hide resolved
@Hoverbear
Copy link
Contributor Author

Thanks for the feedback! Comments resolved. :)

hawkw
hawkw approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

lovely, thanks!

@hawkw hawkw merged commit 9c6dd2d into tokio-rs:master Oct 1, 2020
@jyn514 jyn514 mentioned this pull request Oct 2, 2020
Merged
hawkw pushed a commit that referenced this pull request Oct 6, 2020
@jyn514
chore: remove stdlib.rs (#1008)
dcad8d0 
## Motivation

Since `core` and `alloc` are unconditionally available, there is no
need to use a facade module for most of std.

Follow-up to #960 (comment):
these modules aren't necessary.

## Solution

Most of the imports could be switch to `use core` or `use alloc` without
trouble. The only exceptions were for `Mutex` and `Once`, which are only
available with the full standard library. These had a `sync` module
added to `tracing-core` which acts as a facade, re-exporting `std` if
available and `spin` if not.
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 7, 2020
@dvdplm
Merge remote-tracking branch 'upstream/master' into dp-target-is-cow
381d574 
* upstream/master:
  subscriber: warn if trying to enable a statically disabled level (tokio-rs#990)
  subscriber: use macros for module declarations (tokio-rs#1009)
  chore: remove `stdlib.rs` (tokio-rs#1008)
  core: fix linked list tests reusing `Registration`s (tokio-rs#1016)
  subscriber: support dash in target names (tokio-rs#1012)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003)
  tracing: make `Entered` `!Send` (tokio-rs#1001)
  chore: fix nightly clippy warnings (tokio-rs#991)
  chore: bump all crate versions (tokio-rs#998)
  macros: fix the `tracing-macros` crate not compiling (tokio-rs#1000)
  tracing: prepare to release 0.1.21 (tokio-rs#997)
  core: prepare to release 0.1.17 (tokio-rs#996)
  subscriber: make `PartialOrd` & `Ord` impls more correct (tokio-rs#995)
  core, tracing: don't inline dispatcher::get_default (tokio-rs#994)
  core: make `Level` and `LevelFilter` `Copy` (tokio-rs#992)
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 16, 2020
@dvdplm
Merge remote-tracking branch 'upstream/master' into dp-always-pass-di…
5ad3ab1 
…spatch-as-ref-tokio-rs#455

* upstream/master: (28 commits)
  opentelemetry: prepare for 0.8.0 release (tokio-rs#1036)
  docs: add favicon for extra pretty docs (tokio-rs#1033)
  subscriber: fix `reload` ergonomics (tokio-rs#1035)
  chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031)
  opentelemetry: Assign default ids if missing (tokio-rs#1027)
  chore: remove deprecated add-path from CI (tokio-rs#1026)
  attributes:  fix `#[instrument(err)]` in case of early returns (tokio-rs#1006)
  core: remove mandatory liballoc dependency with no-std (tokio-rs#1017)
  chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023)
  subscriber: warn if trying to enable a statically disabled level (tokio-rs#990)
  subscriber: use macros for module declarations (tokio-rs#1009)
  chore: remove `stdlib.rs` (tokio-rs#1008)
  core: fix linked list tests reusing `Registration`s (tokio-rs#1016)
  subscriber: support dash in target names (tokio-rs#1012)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003)
  tracing: make `Entered` `!Send` (tokio-rs#1001)
  ...
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 19, 2020
@dvdplm
Merge remote-tracking branch 'upstream/master' into dp-fix-tracing-ma…
807e371 
…cros

* upstream/master: (30 commits)
  chore(deps): update pin-project requirement from 0.4 to 1.0 (tokio-rs#1038)
  chore: remove duplicated section from tracing/README.md (tokio-rs#1046)
  opentelemetry: prepare for 0.8.0 release (tokio-rs#1036)
  docs: add favicon for extra pretty docs (tokio-rs#1033)
  subscriber: fix `reload` ergonomics (tokio-rs#1035)
  chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031)
  opentelemetry: Assign default ids if missing (tokio-rs#1027)
  chore: remove deprecated add-path from CI (tokio-rs#1026)
  attributes:  fix `#[instrument(err)]` in case of early returns (tokio-rs#1006)
  core: remove mandatory liballoc dependency with no-std (tokio-rs#1017)
  chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023)
  subscriber: warn if trying to enable a statically disabled level (tokio-rs#990)
  subscriber: use macros for module declarations (tokio-rs#1009)
  chore: remove `stdlib.rs` (tokio-rs#1008)
  core: fix linked list tests reusing `Registration`s (tokio-rs#1016)
  subscriber: support dash in target names (tokio-rs#1012)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@hawkw hawkw hawkw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Hoverbear @hawkw @jyn514