Skip to content

ndk-macro: Remove faulty crate name fallback in fn crate_path()#229

Merged
MarijnS95 merged 1 commit into
masterfrom
ndk-macro-remove-crate-ident-fallback
Feb 15, 2022
Merged

ndk-macro: Remove faulty crate name fallback in fn crate_path()#229
MarijnS95 merged 1 commit into
masterfrom
ndk-macro-remove-crate-ident-fallback

Conversation

@MarijnS95
Copy link
Copy Markdown
Member

proc_macro_crate::crate_name fails to find our ndk-glue crate when it sits inside [target.'cfg(target_os = "android")'.dev-dependencies] as shown in RustAudio/cpal#641, with a fix pending in bkchr/proc-macro-crate#15. This function is only ever called with "ndk-glue" which is an invalid identifier and should have its dash replaced with an underscore to be valid. However, since this fallback doesn't seem to have been hit before (we've never received reports of `"ndk-glue"` is not a valid identifier) it is safe and desired to enforce the crate name to reside in Cargo.toml and proc-macro-crate being able to find it.

`proc_macro_crate::crate_name` fails to find our `ndk-glue`
crate when it sits inside `[target.'cfg(target_os =
"android")'.dev-dependencies]` as shown in [RustAudio/cpal#641], with a
fix pending in [bkchr/proc-macro-crate#15].  This function is only ever
called with `"ndk-glue"` which is an invalid identifier and should have
its dash replaced with an underscore to be valid.  However, since this
fallback doesn't seem to have been hit before (we've never received
reports of `` `"ndk-glue"` is not a valid identifier ``) it is safe and
desired to enforce the crate name to reside in `Cargo.toml` and
`proc-macro-crate` being able to find it.

[RustAudio/cpal#641]: https://github.com/RustAudio/cpal/runs/5203107529?check_suite_focus=true
[bkchr/proc-macro-crate#15]: bkchr/proc-macro-crate#15
@MarijnS95 MarijnS95 force-pushed the ndk-macro-remove-crate-ident-fallback branch from ea664e7 to 57078d9 Compare February 15, 2022 17:20
@MarijnS95 MarijnS95 merged commit d073d47 into master Feb 15, 2022
@MarijnS95 MarijnS95 deleted the ndk-macro-remove-crate-ident-fallback branch February 15, 2022 19:16
@Calsign
Copy link
Copy Markdown

Calsign commented Mar 3, 2022

I encountered this when trying to use this macro in a Bazel build. I was able to work around it by making a dummy Cargo.toml. But just wanted to point out that this isn't a valid assumption to make for users of build systems other than cargo, which is admittedly very few people.

@MarijnS95
Copy link
Copy Markdown
Member Author

@Calsign With "this isn't a valid assumption" I presume you are referring to apk-glue being available in the crate that calls ndk-macro::main?

I've recently had identical troubles with this macro, where I wanted to re-wrap ndk-macro and ndk-glue in another crate to not have to have this and some other boilerplate shared across a bunch of executable crates. Unfortunately this bit requires each of those executable crates to carry ndk-glue in its [dependencies] which isn't ideal.

Perhaps we can re-export ndk-glue from ndk-macro and solve it that way (though ndk-macro may need a proper way to reference $crate).

@Calsign
Copy link
Copy Markdown

Calsign commented Mar 5, 2022

@MarijnS95 Sorry, I was referring to the assumption that "it is safe and desired to enforce the crate name to reside in Cargo.toml and proc-macro-crate being able to find it." In non-cargo build systems there is no Cargo.toml. But I found it is easy to work around this, at least for my case, because ndk_glue::main allows overriding the path to ndk_glue (I didn't realize this at first) which avoids calling into proc-macro-crate at all.

@MarijnS95
Copy link
Copy Markdown
Member Author

@Calsign In that "assumption" the only important bit is that ndk-glue must be in the dependency tree for the calling application (unless it is manually overridden through ndk_glue = "the::path"), and it's "nice" that proc-macro-crate can validate it for us. It wasn't my intention to constrain this exclusively to the cargo build system, yet that's all proc-macro-crate supports.

Do you think the custom ndk_glue = path is good enough for your use-case (I forgot it existed too, that solves my "reexport" problem above 😬 - thanks!), or rather see the right "ndk_glue" instead of "ndk-glue" be put back in place?

@Calsign
Copy link
Copy Markdown

Calsign commented Mar 23, 2022

I think in this case using the override is fine, I just found it very surprising and confusing at first (but this wasn't the first time with trying to use bazel). More broadly I was concerned that other things could rest on the assumption of cargo, and I just wanted to do my small part to make it easier for other to people to use non-cargo build systems in the future.

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.

3 participants