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

Failed loading manifest #137

Closed
fenhl opened this issue May 24, 2024 · 13 comments
Closed

Failed loading manifest #137

fenhl opened this issue May 24, 2024 · 13 comments

Comments

@fenhl
Copy link

fenhl commented May 24, 2024

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
Copy link

Could also be fallout like #135 (comment)? Looks like workspaces weren't tested recently.

@bbqsrc
Copy link
Owner

bbqsrc commented May 27, 2024

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

ianthetechie added a commit to ianthetechie/uniffi-starter that referenced this issue Jun 1, 2024
@ianthetechie
Copy link

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
Copy link
Contributor

mightyguava commented Jun 2, 2024

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
Copy link

@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
Copy link

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
Copy link
Owner

bbqsrc commented Jun 5, 2024

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

@dignifiedquire
Copy link

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

@bbqsrc
Copy link
Owner

bbqsrc commented Aug 19, 2024

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

@mxinden
Copy link

mxinden commented Aug 19, 2024

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: quinn-rs/quinn@7b74ffe

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

cargo-ndk v0.3.4 succeeds:

See commit: quinn-rs/quinn@898cba1

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

Discovered on quinn-rs/quinn#1950.

@bbqsrc
Copy link
Owner

bbqsrc commented Aug 19, 2024

It is yanked!

github-merge-queue bot pushed a commit to n0-computer/iroh that referenced this issue Aug 19, 2024
## Description

cargo-ndk has [known
issues](bbqsrc/cargo-ndk#137) with workspace
setups for versions 3.5.5 and up.

Ensuring docker test runs start with a fresh docker setup.

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [ ] Self-review.
- [ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [ ] All breaking changes documented.
@bbqsrc
Copy link
Owner

bbqsrc commented Aug 21, 2024

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

@mxinden
Copy link

mxinden commented Aug 22, 2024

@bbqsrc main branch works:

(There is an unrelated issue with the emulator.)

rklaehn pushed a commit to n0-computer/iroh-blobs that referenced this issue Oct 22, 2024
## Description

cargo-ndk has [known
issues](bbqsrc/cargo-ndk#137) with workspace
setups for versions 3.5.5 and up.

Ensuring docker test runs start with a fresh docker setup.

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [ ] Self-review.
- [ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [ ] All breaking changes documented.
matheus23 pushed a commit to n0-computer/iroh that referenced this issue Nov 14, 2024
## Description

cargo-ndk has [known
issues](bbqsrc/cargo-ndk#137) with workspace
setups for versions 3.5.5 and up.

Ensuring docker test runs start with a fresh docker setup.

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [ ] Self-review.
- [ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [ ] All breaking changes documented.
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

No branches or pull requests

7 participants