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

Support autobins and target renaming [lib] and [[bin]] #17

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Mar 24, 2022

Fixes rust-mobile/cargo-apk#7

@dvc94ch this WIP PR pulls the lib rename changes out of #15 and expands it with similar changes for [[bin]] and supporting autobins=false. We'll have to rework what Artifacts to return to the user given that by default the library and all binary targets are built: https://doc.rust-lang.org/cargo/commands/cargo-build.html#target-selection. This is different for cargo run which only picks one binary target (I think from the shared pool of src/main.rs, src/bin/*, and any other file specified through [[bin]]) and otherwise errors out.

At the same time I'm thinking about renaming Root to Lib and Bin or otherwise passing such information, so that a caller like cargo-apk knows what it builds (and error accordingly, since cargo-apk always tries to package lib<artifact>.so when it is in fact a binary... resulting in the usual cryptic errors).

src/subcommand.rs Outdated Show resolved Hide resolved
src/subcommand.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Member Author

I found some more peculiar details:

[[bin]]
name = "cargo-subcommand"

This matches the package name and as such uses src/main.rs. Anything else wants:

Caused by:
  can't find `cargo-subcommandx` bin at `src/bin/cargo-subcommandx.rs` or `src/bin/cargo-subcommandx/main.rs`. Please specify bin.path if you want to use a non-default path.

So we should keep that in mind too - for this PR we should probably check for the file paths and error accordingly if it doesn't exist.

@dvc94ch
Copy link
Collaborator

dvc94ch commented Mar 25, 2022

not sure how to proceed here. is this ready for review?

src/artifact.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Member Author

not sure how to proceed here. is this ready for review?

Discuss the review comments I opened, so that I can clean up and finalize this PR. I can also do the more elaborate target selection on my own first, if that's desired. Perhaps we should aim to land #15 first though.

Forgot to mention that this approach doesn't add an extra name: Option to fn file_name() like #15, but instead propagates a rename directly in the Artifact list, is that something you can align with?

@dvc94ch
Copy link
Collaborator

dvc94ch commented Mar 25, 2022

I'd suggest we land #15 first and then see about this one.

@MarijnS95
Copy link
Member Author

@dvc94ch Lovely. That makes my job here much easier, as requested above. Glad to see the name= support was dropped from #15, that should allow us to do both things in isolation :D

@MarijnS95
Copy link
Member Author

I've only rebased this to resolve conflicts for now, we'll need to change Artifact to look something like #15 (comment) so that we can properly deal with detected targets.

Steps are pretty much going to be as follows:

  1. Read all targets explicitly configured in Cargo.toml, validating their existence;
  2. If autoXXX is enabled, merge that with the list of explicitly configured targets;
    (Keeping in mind that any path previously configured in Cargo.toml should NOT be considered as a new, separate autoXXX!)
  3. Filter detected list again based on cmdline arguments, ie. --bin(s)/--lib/--example(s).

Perhaps we can already do the filtering in between step 1 and 2, and then again only run auto target discovery for the target types that were enabled on the cmdline.

Again though, it feels like we're seriously reinventing the wheel here trying to mimic cargo behaviour as closely as possible :( - I really wish cargo-apk and friends could become a plugin to cargo sooner rather than later, where cargo takes care of parsing and passing all the necessary info to us already :/

@quomat
Copy link

quomat commented Mar 19, 2023

@MarijnS95 Maybe using the cargo-metadata crate would help? From the description:

Structured access to the output of cargo metadata. Usually used from within a cargo-* executable.

It feels like a perfect match for cargo-apk. Or is there some roadblock for it?

@MarijnS95
Copy link
Member Author

@quomaz both cargo-apk and cargo-subcommand are pretty much deprecated in favour of xbuild so I don't plan on making too many big changes anymore that would alter the underlying architecture of these projects. Besides, cargo metadata used to be slow (much slower than even repeatedly parsing the same Cargo.toml in this crate) and provides way too much information, that's why the original author of this crate must have opted to write the parsing themselves (even if sticking to cargo semantics becomes harder and harder).

Then there are multiple crates to read information like this, there's also cargo_toml for example.

@MarijnS95 MarijnS95 merged commit 281b281 into master Jun 27, 2023
MarijnS95 added a commit to rust-mobile/cargo-apk that referenced this pull request Jun 27, 2023
MarijnS95 added a commit to rust-mobile/cargo-apk that referenced this pull request Jun 28, 2023
@MarijnS95 MarijnS95 deleted the redo-target-selection branch July 17, 2023 12:33
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.

Library name can be different from the package name Support cargo's library name property
3 participants