rust-mobile / cargo-subcommand

Library for implementing cargo subcommands
6 stars 6 forks source link

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

Closed MarijnS95 closed 1 year ago

MarijnS95 commented 2 years ago

Fixes https://github.com/rust-mobile/cargo-apk/issues/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).

MarijnS95 commented 2 years ago

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 commented 2 years ago

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

MarijnS95 commented 2 years ago

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 commented 2 years ago

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

MarijnS95 commented 2 years ago

@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 commented 2 years ago

I've only rebased this to resolve conflicts for now, we'll need to change Artifact to look something like https://github.com/dvc94ch/cargo-subcommand/pull/15#issuecomment-1078906617 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 commented 1 year ago

@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 commented 1 year ago

@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.