ndk-macro: Reexport android_logger from ndk-glue and use it in macros#88
Conversation
|
Relates to #53 |
|
Thanks for calling this out and making a PR! We always want to improve user experience. I'm definitely in favor of macros being self-contained; implicitly requiring the user to add some deps really sucks.
If we're re-exporting
The only reason
I don't think that's possible? Proc macro crates are pretty limited in what they can export.
We already have prior art in |
I'll remove the path to
Right, they are already supposed to be used together if the rest of the code is believed.
Indeed, I wasn't aware of that. Since you've just made the case for the two crates being heavily tied together there is no reason to move this - it would in fact be impossible which is why this structure is in place already 😊
I see, that was clarified there as well. |
afd08aa to
b285c07
Compare
|
Pretty glad all these nice tests are now actually ran by the CI following #89, makes me feel a lot safer even though I made sure to run most of them locally before pushing :) |
b285c07 to
d1b883f
Compare
I think that seems reasonable. Is compatibility much of a problem here?
That wouldn't actually involve using the |
d1b883f to
771e574
Compare
Agreed there should be no problem in android-ndk using a "random"
It doesn't involve the log crate if one merely wants to configure the logger with
Unfortunately nope. Turns out that's merely a |
771e574 to
59114b9
Compare
|
@francesca64 @msiglreith Is anything left to be discussed here? If I remember correctly (it's been a while) this should be ready for review and merging actually :) |
That's my recollection too! |
This crate does not contain executables nor libraries, just the examples
in the example directory. This is already confusing [1] [2] as they can
only be run with something like:
cargo apk run -p ndk-examples --example hello_world
Or cd-ing into `ndk-examples` before being able to run these.
Unfortunately `cargo apk run -p ndk-examples` keeps working fine,
resulting in:
Error: Path "target/aarch64-linux-android/debug/libndk_examples.so" doesn't exist.
Despite the crate clearly not exposing a library target. The same
happens without this change because `cdylib` is not set for `[lib]`,
but time is still spent compiling all the dependencies instead of
erroring out straight away.
[1]: rust-mobile#48 (comment)
[2]: rust-mobile#29 (comment)
It is more common to use a logging framework instead of printing raw text to stdout that ends up in the logs; lead by example. More importantly this exemplifies a current issue with android_logger not being exported by the relevant macro crate.
When enabling the logger feature:
error[E0433]: failed to resolve: use of undeclared type or module `android_logger`
--> ndk-examples/examples/hello_world.rs:6:5
|
6 | ndk_glue::main(backtrace = "on", logger(level = "debug", tag = "hello-world"))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of undeclared type or module `android_logger`
|
= note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)
This means the crate user is responsible for importing the
android_logger crate in their namespace where this macro is used. That
is not ideal, especially when ndk-glue already imports it but doesn't
seem to use it anywhere.
In accordance with a prior, identical change for android_logger: reexport the log crate from ndk-glue and use it in the macro instead of relying on crate users to provide log in this scope or override the path. This is especially useful if a crate wishes to configure the log level or tag without needing the log crate in the scope where ndk_glue::main is called.
59114b9 to
7f9eeb8
Compare
Cool! Are you in a position to give this a quick read-through and review? |
|
Documented the changes in |
|
Thanks for the review all! |
Hi all,
Upfront apologies for my bluntness in this "RFC": I'm usually rather disappointed after running into
undeclared type or moduleerrors when a macro generates code in my (programs) scope with a hard dependency on some crate or symbol there.This PR is a suggested "fix" for addressing the missing
android_loggerlogger when enabling theloggingfeature, without importing that crate into the target package. It is not how I envision a final solution so I'm opening this for feedback, read on for more questions. Note that tests were intentionally not updated until a proper discussion how to best implement this.I haven't really understood the purpose of providing the path to
android_loggerin the macro; it is already linked byndk-glueand is best used from there. If this PR finds its way to master one way or another, how about removing this "path override" feature, as there should now be one consistent place to referenceandroid_loggerfrom? We might givelogthe same treatment, even though this is generally available in the same scope to print log messages.ndk-gluedoesn't seem to useandroid_loggerat all, does it make more sense to move this dependency intondk-macroso that it can be used on its own? There's no$crate::android_loggerin proc-macro to make this any easier though.