bbqsrc / cargo-ndk

Compile Rust projects against the Android NDK without hassle
Apache License 2.0
712 stars 64 forks source link

Failed loading manifest #137

Closed fenhl closed 6 months ago

fenhl commented 6 months ago

I have a package night-uniffi that's part of a virtual workspace. With cargo-ndk 3.5.4, everything works fine. With cargo-ndk 3.5.6, the command cargo ndk --platform=34 --target=arm64-v8a build --package=night-uniffi --lib errors with the following output:

error: Failed loading manifest
error: Could not derive library name from Cargo.toml
MarijnS95 commented 6 months ago

Could also be fallout like https://github.com/bbqsrc/cargo-ndk/pull/135#discussion_r1606097922? Looks like workspaces weren't tested recently.

bbqsrc commented 6 months ago

It is likely that. Won't have time to look into this for a while unfortunately, so contributions here are welcomed.

ianthetechie commented 6 months ago

Hmm, that's unfortunate. I hit this in a CI pipeline as well which pulled the latest patch release. I use virtual workspaces across several projects, and this thread has been helpful understanding the issue. As for solutions, let's see if I can get my head around the factors.

First, it looks like the changes introduced are aimed at avoiding copying extra libraries which are unneeded/harmful. It tries to do so by looking at Cargo.toml.

As far as I can tell, every rust crate can have 0 or 1 libraries, and package.name is required in Cargo.toml so we always have a name

@mightyguava wrote the above, and it's mostly correct. Every rust package can have 0 or 1 libraries, per the cargo book. However, the code as-is assumes that we are parsing a manifest for a single package rather than a workspace, which may contain multiple packages.

I'm not sure that the current model's assumptions hold for more complex projects. In the simple case, there is only one library, so you just recursively parse the workspace member manifests and then get the library/package name. That would work for my use case, but I feel there's something missing for the general case.

Thoughts?

mightyguava commented 6 months ago

How are you invoking cargo-ndk? We also use workspaces, but our build scripts invoke the build for a single package at a time with the --package flag.

We never run the build at the workspace level. Does that automatically build all packages in the workspace? If so, then maybe cargo-ndk may want to revert to the old behavior of copying the whole target directory on build in this scenario... or probably better, parse all member manifests and copy every lib found.

MarijnS95 commented 6 months ago

@mightyguava see https://doc.rust-lang.org/cargo/reference/workspaces.html#the-default-members-field. Afaik this defaults to the root [package] if the root Cargo.toml defines one, and no default-members are declared.

ianthetechie commented 6 months ago

How are you invoking cargo-ndk? We also use workspaces, but our build scripts invoke the build for a single package at a time with the --package flag.

My use case is perhaps similar to @fenhl's as I'm using UniFFI. While there is more than one way to do it, I found that it's easiest to use a gradle plugin. Here is my invocation.

We never run the build at the workspace level. Does that automatically build all packages in the workspace? If so, then maybe cargo-ndk may want to revert to the old behavior of copying the whole target directory on build in this scenario... or probably better, parse all member manifests and copy every lib found.

@MarijnS95's answer is correct. However, you raise an interesting idea in the first half of your comment. It looks like the gradle plugin lets you pass arbitrary cargo args. It looks like I can pass extraCargoBuildArguments = ["-p", "ferrostar"] to fix it!

I don't know if this will work if you need to bundle multiple libraries, but it works for my use case.

bbqsrc commented 6 months ago

I have yanked 3.5.5 and 3.5.6. This'll have to be revisited in 3.6.x or later.

dignifiedquire commented 3 months ago

why is this closed? this still happens for us on 3.5.7

bbqsrc commented 3 months ago

Ah shit, yeah the offending changes were never reverted. I'll yank this too lol

mxinden commented 3 months ago

First off, thank you for this excellent crate!

why is this closed? this still happens for us on 3.5.7

Same on quinn CI:

cargo-ndk v0.3.7 fails with:

error: Failed loading manifest
error: Could not derive library name from Cargo.toml

See commit: https://github.com/quinn-rs/quinn/commit/7b74ffe2a0f0de47bb112f5009418ac748ab6a94

See GitHub Action run: https://github.com/quinn-rs/quinn/actions/runs/10450996767/job/28936466966

cargo-ndk v0.3.4 succeeds:

See commit: https://github.com/quinn-rs/quinn/commit/898cba1bd709a1582e24a01108c4fc11fcc5259e

See GitHub Action run: https://github.com/quinn-rs/quinn/actions/runs/10451996839/job/28939531590?pr=1950

Discovered on https://github.com/quinn-rs/quinn/pull/1950.

bbqsrc commented 3 months ago

It is yanked!

bbqsrc commented 3 months ago

@mxinden @dignifiedquire could you try with the main branch and let me know if the current state of things works for you?

mxinden commented 3 months ago

@bbqsrc main branch works:

(There is an unrelated issue with the emulator.)