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

melpomene: make tracing feature flags additive #1

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jul 7, 2022

Currently, the tracing behavior in Melpomene is controlled by a pair
of feature flags, "trace-modality" and "trace-fmt". These select between
emitting traces to stdout using tracing_subscriber::fmt, and emitting
to Modality using the tracing-modality crate. However, these feature
flags are not additive. If they are both enabled, the code will not
compile, since there are two implementations of the setup_tracing
function, which are conditionally compiled based on whether those
feature flags are enabled. If both are enabled, both functions will be
present, resulting in a compiler error.

This commit changes this code so that there is a single implementation
of the setup_tracing function, which will construct a tracing
subscriber that includes either fmt, Modality, or both, depending on
the feature combination. Now, Melpomene should build even with
--all-features or similar.

Now that the fmt layer is built manually rather than using
fmt::init() "easy mode" helper, I've also changed the filter
environment variable from "RUST_LOG" to "MELPOMENE_TRACE". I thought
this was nicer, but we can continue using the default env var if that's
preferable.

Currently, the `tracing` behavior in Melpomene is controlled by a pair
of feature flags, "trace-modality" and "trace-fmt". These select between
emitting traces to stdout using `tracing_subscriber::fmt`, and emitting
to Modality using the `tracing-modality` crate. However, these feature
flags are not additive. If they are both enabled, the code will not
compile, since there are two implementations of the `setup_tracing`
function, which are conditionally compiled based on whether those
feature flags are enabled. If both are enabled, both functions will be
present, resulting in a compiler error.

This commit changes this code so that there is a single implementation
of the `setup_tracing` function, which will construct a `tracing`
subscriber that includes either `fmt`, Modality, or _both_, depending on
the feature combination. Now, Melpomene should build even with
`--all-features` or similar.

Now that the `fmt` layer is built manually rather than using
`fmt::init()` "easy mode" helper, I've also changed the filter
environment variable from "RUST_LOG" to "MELPOMENE_TRACE". I thought
this was nicer, but we can continue using the default env var if that's
preferable.
hawkw added a commit that referenced this pull request Jul 7, 2022
This branch makes the tracing configuration slightly nicer (IMO).
Depends on #1.
@jamesmunns
Copy link
Contributor

Tested this works with both subscribers! Thanks :)

@jamesmunns jamesmunns merged commit 98753bb into tosc-rs:main Jul 7, 2022
jamesmunns added a commit that referenced this pull request Jun 4, 2023
Add `fmt` trait impls for heap containers
jamesmunns added a commit that referenced this pull request Jun 4, 2023
Merge commit. Introduce async variant of the vm.

Intended to be used for cases where the host can provide non-blocking words,
which by yielding with async, can avoid busy loops in environments without
threading/preemptive multitasking.

Original commits:

* refactor: use `Poll` rather than `Done` enum
* rough implementation of async execution
* sickos YES
* Rework to handle dispatch more like builtins (but defer exec)
* Introduce a dedicated AsyncBuiltinEntry type
* Restore test
* Write a test, fix some bugs
* allow async builtin futures to borrow `Forth`

This way, the async builtin can mutate the forth stacks when it
completes, which is necessary to actually return values from async
builtins.

* fully feature-flag async stuff
* add a separate `AsyncForth` API type

This introduces some unfortunate code duplication, but it lets us
statically prevent errors resulting from async steps performed by
non-async VMs and vice versa. It also lets us enforce that the same
async dispatcher is used for the whole lifetime of the VM, which will
prevent potential bugs from calling a step with an async dispatcher
that doesn't know the set of builtins registered by the VM.

* make the dispatcher provide the list of builtins
* document async builtins
* document sync builtins
* rm unused dep

---------

Co-authored-by: James Munns <james@onevariable.com>
@jamesmunns jamesmunns added platform: melpomene Specific to the Melpomene simulator platform area: tools & build Related to host developer tools, including tracing, Crowtty and build processes labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tools & build Related to host developer tools, including tracing, Crowtty and build processes platform: melpomene Specific to the Melpomene simulator platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants