Skip to content

Enable test-only code via feature instead of target_arch.#291

Merged
japaric merged 3 commits intoknurling-rs:mainfrom
Dirbaio:x86
Dec 1, 2020
Merged

Enable test-only code via feature instead of target_arch.#291
japaric merged 3 commits intoknurling-rs:mainfrom
Dirbaio:x86

Conversation

@Dirbaio
Copy link
Copy Markdown
Contributor

@Dirbaio Dirbaio commented Nov 30, 2020

Fixes #223

  • Replace target_arch = "x86_64" with feature = "unstable-test" everywhere
  • Add features to Cargo.tomls
  • Add --features unstable-test in CI script

Copy link
Copy Markdown
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

The approach looks good to me.

I think you meant to put the cfg instead the quote! macros though. See my inline comments.

Comment thread macros/Cargo.toml

[features]
# WARNING: for internal use only, not covered by semver guarantees
unstable-test = []
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think the feature is necessary in this crate. The proc macro code is emitting cfg(feature = "unstable-test") but not using the Cargo feature.

EDIT: after reading the rest of the PR, I see that we do want this

Comment thread macros/src/lib.rs Outdated

quote!(match () {
#[cfg(target_arch = "x86_64")]
#[cfg(feature = "unstable-test")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this (cfg inside quote!) seems fine because internp is an implementation detail and only used within defmt non-public / non-user-facing code.

Comment thread macros/src/lib.rs Outdated
};
quote!(match () {
#[cfg(target_arch = "x86_64")]
#[cfg(feature = "unstable-test")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this cfg however will end in the code emitted by the logging macros, write!, intern!, etc. I'm not sure if that's the behavior we want.

The other option is make the defmt macros always emit the fetch_add_string_index version if its "unstable-test" feature is enabled.

EDIT: OK, after reading the rest of the PR. I think this should be:

if cfg!(feature = "unstable-test") {
    quote!(/* .. */) // fetch add string version
} else {
    quote!(/* .. */) // linker version
}

Comment thread src/lib.rs Outdated
// (using `core::unreachable!` instead of `unreachable!` doesn't help)
#[cfg(target_arch = "x86_64")]
#[cfg(feature = "unstable-test")]
mod x86_64 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: this module should be renamed as it's no longer x86 only

Comment thread .github/workflows/ci.yml
- name: Run tests on Ubuntu/Windows
if: matrix.os != 'macOS-latest'
run: cargo test --workspace
run: cargo test --workspace --features unstable-test
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in addition to this I think we should also run: RUSTFLAGS='--deny warnings' cargo check --features unstable-test (there's a previous build step that does this without the --features)

@Dirbaio
Copy link
Copy Markdown
Contributor Author

Dirbaio commented Nov 30, 2020

Thanks for the review. Nice catch about the macros, I didn't think about that at all.
Review comments adressed, plus I fixed some comments and docs to reflect that defmt is now usable on x64.

Copy link
Copy Markdown
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

This is great 💯 Thanks for implementing this!

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.

Allow using on x86_64

2 participants