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

Remove stdlib.rs #1008

Merged
merged 2 commits into from
Oct 6, 2020
Merged

Remove stdlib.rs #1008

merged 2 commits into from
Oct 6, 2020

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Oct 2, 2020

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.

r? @hawkw

@jyn514 jyn514 requested review from hawkw and a team as code owners October 2, 2020 02:25
hawkw
hawkw approved these changes Oct 2, 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.

looks good to me --- i had one question about the extern crate decl for liballoc.

tracing-core/src/lib.rs Show resolved Hide resolved
tracing-futures/src/lib.rs
@@ -105,10 +105,8 @@
#[cfg(feature = "std-future")]
use pin_project::pin_project;

pub(crate) mod stdlib;

#[cfg(feature = "std-future")]
Copy link
Member

Choose a reason for hiding this comment

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

we can probably just remove the feature flag for std::future in the upcoming release.... it was added way back in the day before std::future was stable, so it could be enabled by users on nightly. now, it's stable in our MSRV, so this is definitely no longer necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to remove all the uses of the feature here? I can keep std-future = [] as a no-op so it doesn't break existing code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually it looks like enabling std-future conflicts with futures_01 ... do you plan to remove futures_01 in tracing 0.2?

@hawkw
Copy link
Member

hawkw commented Oct 2, 2020

looks like there are some merge conflicts, and rustfmt needs to be run?

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 3, 2020

I rebased and ran rustfmt. I'm going to remove the std-future feature as mentioned in #1008 (comment), let me know if I should push it here or as a seperate PR.

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 3, 2020

Hmm, this doesn't look related to my change.

---- callsite::tests::linked_list_push stdout ----
thread 'callsite::tests::linked_list_push' panicked at 'attempt to add with overflow', tracing-core/src/callsite.rs:322:13

@hawkw
Copy link
Member

hawkw commented Oct 5, 2020

Hmm, this doesn't look related to my change.

---- callsite::tests::linked_list_push stdout ----
thread 'callsite::tests::linked_list_push' panicked at 'attempt to add with overflow', tracing-core/src/callsite.rs:322:13

Hmm, that test was added quite recently (in PR #988), so I'm curious if there's a flaky behavior in the test, or a racy bug that only occurs occasionally. I'm going to take a closer look at that.

@hawkw hawkw mentioned this pull request Oct 5, 2020
Merged
hawkw added a commit that referenced this pull request Oct 5, 2020
@hawkw
core: fix linked list tests reusing Registrations (#1016)
4707e6f 
## Motivation

Currently, the tests for the linked list implementation in
`tracing_core::callsite` declare two static `Registration`s in the test
module, which are used in multiple tests. Since these tests are all run
in one binary, in separate threads, these statics are shared across all
tests. This means that --- depending on the order in which tests are run
--- these `Registration`s may already have values for their `next`
pointers. In particular, there's a potential issue where the `for_each`
in the test `linked_list_push` can loop repeatedly, if one of the
`Registration`s already had a next pointer from a previous test. See:
#1008 (comment)

## Solution

This branch declares separate `Registration` statics for each test.
These are not shared between multiple test threads.

We still reuse the same callsite statics, since their state does not
change during the tests --- only the `Registration`s change. I also
refactored the test code a little bit to use only a single type
implementing `Callsite`, rather than two separate types, since the
callsite impl is not actually used and this makes the code somewhat more
concise.

Finally, I added a new test for pushing more than two callsites.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member

hawkw commented Oct 5, 2020

Hmm, this doesn't look related to my change.

---- callsite::tests::linked_list_push stdout ----
thread 'callsite::tests::linked_list_push' panicked at 'attempt to add with overflow', tracing-core/src/callsite.rs:322:13

okay, #1016 fixes this --- mind rebasing? this should be good to merge once we get a green build :)

@jyn514
Remove stdlib.rs
bb87e38 
Since `core` and `alloc` are unconditionally available, there was no
need to use a facade module for most of std. 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.
@jyn514
cargo fmt
c75c5b6
@jyn514
Copy link
Contributor Author

jyn514 commented Oct 6, 2020

Rebased and CI is passing :)

hawkw
hawkw approved these changes Oct 6, 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.

okay, this seems good to me!

@hawkw hawkw merged commit dcad8d0 into tokio-rs:master Oct 6, 2020
@hawkw
Copy link
Member

hawkw commented Oct 6, 2020

This should hopefully make #1017 a bit nicer as well. :)

@jyn514 jyn514 deleted the cfg branch October 6, 2020 00:40
@jyn514
Copy link
Contributor Author

jyn514 commented Oct 6, 2020

Awesome! Glad I could help :)

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)
  ...
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 19, 2020
@dvdplm
Merge remote-tracking branch 'upstream/master' into dp-remove-TraceLo…
276868d 
…gger

* upstream/master: (31 commits)
  chore: fix tracing-macros::dbg (tokio-rs#1054)
  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)
  ...
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 22, 2020
@dvdplm
Merge remote-tracking branch 'upstream/master' into dp-always-pass-di…
bd22923 
…spatch-as-owned-tokio-rs#455

* upstream/master: (34 commits)
  subscriber: remove TraceLogger (tokio-rs#1052)
  subscriber: make Registry::enter/exit much faster (tokio-rs#1058)
  chore(deps): update env_logger requirement from 0.7 to 0.8 (tokio-rs#1050)
  chore: fix tracing-macros::dbg (tokio-rs#1054)
  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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

None yet
2 participants
@jyn514 @hawkw