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

core: vendor lazy_static and spin for no_std support #424

Merged
merged 9 commits into from
Nov 16, 2019

Conversation

thombles
Copy link
Contributor

@thombles thombles commented Nov 14, 2019

Following the suggested fix on #365: #365 (comment)

tracing-core's use of spin is feature-gated. lazy_static is vendored for either feature but I've modified it to respect our feature flags. I've also removed doc comments that no longer compile and suppressed a couple of warnings with the lints that are now being applied.

At this point

  • Including this in another project with std lazy_static works
  • Tracing unit tests pass both with and without --no-default-features

However I feel like a bull in a china closet, copying in gobs of dependencies to an unfamiliar project. Feedback about how best to arrange this will be greatly appreciated!

Fixes #365

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.

Thanks so much for working on this, I really appreciate it!

I noticed a few things that will need to be addressed before we can merge this — in particular, I think the doctests in the vendored code is breaking the build, and we need to be able to reexport a Once type in tracing for the macros to work correctly. Additionally, I think it's important that the vendored code not be publicly accessible to user code, or at least doc(hidden) when it has to be reexported.

tracing-core/src/lib.rs Outdated Show resolved Hide resolved
tracing-core/src/lib.rs Outdated Show resolved Hide resolved
tracing-core/src/spin/mutex.rs Outdated Show resolved Hide resolved
tracing-core/src/spin/mutex.rs Outdated Show resolved Hide resolved
tracing-core/src/spin/mutex.rs Outdated Show resolved Hide resolved
tracing-core/src/spin/once.rs Outdated Show resolved Hide resolved
tracing-core/src/spin/once.rs Outdated Show resolved Hide resolved
tracing/Cargo.toml Show resolved Hide resolved
tracing-core/src/lib.rs Outdated Show resolved Hide resolved
tracing-core/src/lib.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Nov 14, 2019

Also, would you mind adding "Fixes #365" to the PR description before we merge this? Thanks!

@thombles
Copy link
Contributor Author

Apologies, my first attempt was far more broken than I realised! I tripped over rust-lang/cargo#5015 - I thought I was building without std but I wasn't. Thanks for the detailed feedback!

In the end I removed all the spin tests as they depend on std. The lints helped me prune out the things we weren't using. My first thought was to leave the vendored code as close to upstream as possible but I've been nudged towards just including what we need.

My only lingering concern is that tracing points at tracing-core 0.1.7, which lacks our new Once export. I confirmed it builds if I set the path manually. I guess it will be okay if they're released in sync. Do I need to do anything about it for this PR?

@hawkw
Copy link
Member

hawkw commented Nov 15, 2019

My only lingering concern is that tracing points at tracing-core 0.1.7, which lacks our new Once export. I confirmed it builds if I set the path manually. I guess it will be okay if they're released in sync. Do I need to do anything about it for this PR?

Yes, it would be great if you would do the following:

  • bump tracing-core's version to 0.1.8
  • set thetracing-core dependency version in tracing's Cargo.toml to version 0.1.8
  • add path = "../tracing-core"that dependency

Then everything should build successfully on CI, and we can publish a new version of tracing-core after this merges.

@hawkw
Copy link
Member

hawkw commented Nov 15, 2019

Hmm, actually...tracing-core doesn't actually use spin::Once, right? Just `spin::Mutex?

If that's the case, can we just move the vendored spin::Once into tracing, and leave spin::Mutex in tracing-core? That way, we don't need to bump the version dependency on tracing-core. I think this would be a better approach, since there isn't really any reason the vendored Once needs to be in tracing-core...

@thombles
Copy link
Contributor Author

thombles commented Nov 15, 2019

Hmm, actually...tracing-core doesn't actually use spin::Once, right? Just `spin::Mutex?

tracing-core doesn't directly, but the spin variant of the vendored lazy_static does unfortunately.

I guess another possibility is to let tracing continue to depend on crates.io spin, independently of the vendored Once in tracing-core. Everyone's been compiling it until now anyway. Would that be better, or should I go for the version bump?

@hawkw
Copy link
Member

hawkw commented Nov 15, 2019

Hmm. tracing still needs some kind of no-std compatible Once. I guess let's move forward with the version bump...it seems less bad than vendoring two separate versions of Once...

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 looks good to me. thanks so much for working on this!

@hawkw hawkw merged commit 5a4da14 into tokio-rs:master Nov 16, 2019
hawkw added a commit that referenced this pull request Dec 20, 2019
Added

- `Default` impl for `Dispatch` (#411)

Fixed

- Removed duplicate `lazy_static` dependencies (#424)
- Fixed no-std dependencies being enabled even when `std` feature flag
  is set (#424)
- Broken link to `Metadata` in `Event` docs (#461)

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this pull request Dec 20, 2019
Added

- `Default` impl for `Dispatch` (#411)

Fixed

- Removed duplicate `lazy_static` dependencies (#424)
- Fixed no-std dependencies being enabled even when `std` feature flag
  is set (#424)
- Broken link to `Metadata` in `Event` docs (#461)

Signed-off-by: Eliza Weisman <[email protected]>
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.

Feature-dependent dependencies are documented to not work
2 participants