Read environment variables from .cargo/config.toml's [env] section

[MarijnS95]

Closes #230

Environment variables can be set and optionally override the process environment through .cargo/config.toml's [env] section: https://doc.rust-lang.org/cargo/reference/config.html#env

These config variables have specific precedence rules with regards to overriding the environment set in the process, and can optionally represent paths relative to the parent of the containing .cargo/ folder. The entire handling of these variables is implemented in rust-mobile/cargo-subcommand#12 which can be read as-is from cargo-apk and ndk-build, through a trait to keep ndk-build oblivious for cargo-subcommand.

[msiglreith]

intergrating the queried key into the error msg would be helpful imo

[dvc94ch]

mmh, don't necessarily like that this env thing needs to be passed in to the Ndk. wouldn't it be better to do what cargo does and have cargo-subcommand set the environment variables?

[dvc94ch]

sorry for being a bit late to the game, hadn't looked at this PR yet.

[MarijnS95]

mmh, don't necessarily like that this env thing needs to be passed in to the Ndk. wouldn't it be better to do what cargo does and have cargo-subcommand set the environment variables?

I hadn't really even considered that. Is it something we'd like cargo-subcommand to do instead of forcing everyone to implement this environment search?

EDIT: That way this PR likely becomes a no-op and can be closed.

[dvc94ch]

I think so yes. I still think that down the road we want to remove arg parsing and instead have something called CargoWorkspace that could expose something like set_env or artifact returning the path of an artifact and whatever other useful stuf cargo-subcommand does.

[dvc94ch]

Wondering what happens if you run the algorithm twice. set_env is idempotent? Running it twice will give the same result as running it once? cargo build will perform the setup again, so we don't want the results to be altered unexpectedly

[MarijnS95]

set_env should be idempotent, and we won't modify any data in the environment, just overwrite it. Updating the process environment with stuff from [env] in cargo-subcommand should also be idempotent, though I'm curious how merging multiple .configs works, since you can then have different definitions for force=true (was it set in the process environment from the get-go, or because a previous .config set it?). That's probably documented though, will read it when we get to it.

[MarijnS95]

I still think that down the road we want to remove arg parsing

Isn't cargo-subcommand still quite heavily reliant on cmdline arguments? Do we then let the caller parse it and pass us the arguments, or we parse the cmdline in cargo-subcommand and let the caller parse it for themselves?

[dvc94ch]

well we can have a config struct with all the cmdline arguments. Something like this:

#[cfg_attr(feature = "clap", derive(Parser))]
struct Args {
    target_dir: Option<PathBuf>,
    package: Option<String>,
    examples: Option<bool>,
    example: Option<String>,
}

If a subcommand uses clap and uses all the supported args it can just #[clap(flatten)] cargo_args: cargo_subcommand::Args and otherwise just construct the Args struct manually.

[dvc94ch]

Closing as per discussion

[MarijnS95]

@dvc94ch Yeah that seems like a really neat solution, but isn't really relevant for this PR.

Not sure about preliminarily closing this before we have implemented the set_env() alternative in cargo-subcommand though, but I won't be able to get to it for a while unfortunately :(

[dvc94ch]

I guess we can keep it open for now

[MarijnS95]

This is finally unnecessary now that rust-mobile/cargo-subcommand#16 is open 🥳!