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

Consistent cli #14

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@gibix
Contributor

gibix commented Jul 3, 2018

No description provided.

@cardoe

I honestly don't see the point of the first commit. If the goal is to have multiple sub-commands then they should be parsed out in the binary and there should be multiple entry points in the library instead of feeding it all into a run() function in the library and having it parse options again. This makes the code much more testable. Half the changes in this commit focuses on functionality that's removed in the very next commit. This commit is dense and could have a few commits:

  • add colorized help for a better user experience
  • remove unnecessary clap settings
  • multiple entry points

The second commit is very dense that could honestly be broken up into a few logical pieces. But ultimately it breaks fallback cases and produces invalid ebuilds without reporting anything to the user.

  • code style changes
  • cargo -> cargo_metadata

The third commit starts using the log crate to let the user know that the ebuild is now invalid but still causes success to be returned back to the user. Overall if we're going to produce an invalid ebuild then we should immediately stop and error out and not exit with a ret code of 0.

src/lib.rs Outdated
let name = read_from_manifest(package, &"name").unwrap_or_else(|| String::from(""));
let license = read_from_manifest(package, &"license").unwrap_or_else(|| String::from(""));
let description = read_from_manifest(package, &"description").unwrap_or_else(|| name.clone());
let homepage = read_from_manifest(package, &"homepage").unwrap_or_else(|| String::from(""));

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

So the old code used the repository field if there was no homepage and this just uses an empty string.

src/lib.rs Outdated
.get("package")
.ok_or_else(|| format_err!("Field \"package\" is missing in Cargo manifest"))?;
let name = read_from_manifest(package, &"name").unwrap_or_else(|| String::from(""));
let license = read_from_manifest(package, &"license").unwrap_or_else(|| String::from(""));

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

The old code understood license-file and license while this understands only the later and produces an invalid ebuild

This comment has been minimized.

@gibix

gibix Jul 8, 2018

Contributor

I missed in the changes, sorry.

src/lib.rs Outdated
.ok_or_else(|| format_err!("Field \"package\" is missing in Cargo manifest"))?;
let name = read_from_manifest(package, &"name").unwrap_or_else(|| String::from(""));
let license = read_from_manifest(package, &"license").unwrap_or_else(|| String::from(""));
let description = read_from_manifest(package, &"description").unwrap_or_else(|| name.clone());

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

The old code had a fallback case that's gone.

src/lib.rs Outdated
// build up the ebuild path
let ebuild_path = PathBuf::from(format!("{}-{}.ebuild", package.name(), package.version()));
let path = PathBuf::from(ebuild_path.unwrap_or_else(|| format!("{}-{}.ebuild", name, version)));

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

Wouldn't a better fallback be to the current directory so that the code below could be used as the fallback. Instead there's two failure cases to handle. I would clean this up.

src/lib.rs Outdated
.write(true)
.create(true)
.truncate(true)
.open(&ebuild_path)?;

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

ok this is a trivial code style change and I can break it out of this larger change.

This comment has been minimized.

@gibix

gibix Jul 8, 2018

Contributor

just cargo fmt. should be mandatory before commit

src/lib.rs Outdated
fn read_from_manifest(package: &Value, query: &str) -> Option<String> {
package
.get(query)
.unwrap()

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

Is this always going to succeed?

This comment has been minimized.

@gibix

gibix Jul 8, 2018

Contributor

I want to improve this query, now is possible that panics

src/lib.rs Outdated
}
/// Parse cli commands
pub fn run(cmd: Option<Command>, verbose: u32, quiet: bool) -> CliResult {

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

So you remove the verbose and quiet flags after making the prior commit mostly about them?

This comment has been minimized.

@gibix

gibix Jul 8, 2018

Contributor

the prior commit had nothing to do with verbosity and quiet

src/lib.rs Outdated
use cargo::ops;
use cargo::util::{important_paths, CargoResult, CargoResultExt};
use cargo::{CliResult, Config};
pub use failure::Error; // re-exported to main

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

Why? Main can't just pull in its own version.

This comment has been minimized.

@gibix

gibix Jul 8, 2018

Contributor

You are asking why I'm not declaring it also in the main? Because in main I'm not using logging macro if so I will update it.

This comment has been minimized.

@cardoe

cardoe Aug 25, 2018

Owner

No asking why you're reexporting this which makes it part of the ABI.

src/main.rs Outdated
// run the actual code
if let Err(e) = run(opt.verbose as u32, opt.quiet) {
if let Err(e) = run(opt.cmd, opt.verbose as u32, opt.quiet) {

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

This commit is mostly centered around this change. But then its removed in the next commit.

This comment has been minimized.

@gibix

gibix Jul 8, 2018

Contributor

here verbosity and quiet are used in the library, in future changes not.

src/lib.rs Outdated
@@ -21,6 +23,24 @@ use std::fs::OpenOptions;
use std::io::Write;
use std::path::PathBuf;
#[derive(Debug, StructOpt)]

This comment has been minimized.

@cardoe

cardoe Jul 8, 2018

Owner

I don't see why we would put an option parser into the library portion. The main function is where option parsing belongs.

@gibix

This comment has been minimized.

Contributor

gibix commented Jul 8, 2018

I don't agree with your idea of apis, I like to have a single run function that can handle all the commands and options. Most of the tools in rust that I saw use that logic and I tried to be close to that. Specially because there are multiple features that I want to add in future.
By the way I don't want to keep this discussion further and I will adopt it as the commit splitting.

The log is used to warn that some fields in the manifest were missing. Anyway as user I still want have the ebuild generated to be able to fix it by hand or investigate better.

gibix added some commits Jul 17, 2018

add colorized help for a better user experience
setup clap flag to have colored help message

Signed-off-by: gibix <gibix@riseup.net>
remove unnecessary clap settings
Fix some clap settings are useless or default value

Signed-off-by: gibix <gibix@riseup.net>
multiple entry points
- Add support for multiple commands
- The `run` function in the lib now can easily take care of defaults and
  command parsing. This should make adding new features and flags easier
- verbosity and quiet flags now are showed correctly in the help message

Signed-off-by: gibix <gibix@riseup.net>
drop cargo for cargo_metadata
Cargo_metadata is a crate the expose the cargo-metadata subcommand. Rely
on metadata and not on cargo directly reduces considerably both the
runtime and compile-time costs.  The downside is that cargo_metadata
doesn't expose cargo's internal types and metadata. By now is not a
blocking issue because the missing informations can be extracted
directly from the manifest file.  Has been added also another argument
to the `build` to specify a manifest path, because of that now
cargo-ebuild doesn't rely anymore on a cargo project but only on the
manifest.  This change removes also some boilerplate code like resolve,
workspace, etc.

In future work should ported the [package.metadata] field of the
`Cargo.toml` to handle cargo-ebuild specific configurations (es
KEYWORDS).

In future new version of the cargo-metadata format and cargo features
should be mapped into cargo_metadata in order to be used.

Signed-off-by: gibix <gibix@riseup.net>
add basic logging
Use log in the library for debugging and error message, the logger is
setted up in the binary to make the library agnostic for every kind of
log builder.

Signed-off-by: gibix <gibix@riseup.net>

@gibix gibix force-pushed the gibix:consistent-cli branch from 42df0a3 to 5e7377b Jul 17, 2018

gibix added some commits Jul 25, 2018

Add cargo-features support
Extract list of features from the cargo project and write them in the
ebuild as CARGO_FEATURES. This will be used by cargo.eclass to compile the
project with specific features.
add correct license handling
The license field in the cargo manifest can be defined by '/' separated
license (deprecated) or with "AND/OR" grammar. Has been added the support
to both cases.

@gibix gibix referenced this pull request Jul 26, 2018

Closed

rust eclass for multi-target #362

@gibix

This comment has been minimized.

Contributor

gibix commented Aug 24, 2018

Ehi I think that at least the use as flag part can be cherry picked

@cardoe

First patch doesn't do what it intends and the other change contained (and not mentioned in the commit message) has an unintended consequence of breaking the help output.

src/main.rs Outdated
@@ -30,7 +30,7 @@ struct Args {
}
#[derive(StructOpt, Debug)]
#[structopt(bin_name = "cargo")]
#[structopt(name = "cargo", raw(setting = "AppSettings::ColoredHelp"))]

This comment has been minimized.

@cardoe

cardoe Aug 26, 2018

Owner

Prior to this change (there is no color):

$ ./target/debug/cargo-ebuild ebuild --help
cargo-ebuild 0.1.6-pre
Doug Goldstein <cardoe@cardoe.com>
Generates an ebuild for a given Cargo project

USAGE:
    cargo ebuild [OPTIONS]

OPTIONS:
    -q, --quiet      Silence all output
    -v, --verbose    Verbose mode (-v, -vv, -vvv, etc.)
    -h, --help       Prints help information
    -V, --version    Prints version information

With this change (there is still no color):

$ ./target/debug/cargo-ebuild ebuild --help
cargo-ebuild-ebuild 0.1.6-pre
Doug Goldstein <cardoe@cardoe.com>
Generates an ebuild for a given Cargo project

USAGE:
    cargo-ebuild ebuild [OPTIONS]

OPTIONS:
    -q, --quiet      Silence all output
    -v, --verbose    Verbose mode (-v, -vv, -vvv, etc.)
    -h, --help       Prints help information
    -V, --version    Prints version information

So not only does this not add color but it breaks the help output. I would drop this patch.

src/main.rs Outdated
setting = "AppSettings::DontCollapseArgsInUsage"
)
)]
#[structopt( name = "ebuild",)]

This comment has been minimized.

@cardoe

cardoe Aug 26, 2018

Owner

Trailing comma

@cardoe

This comment has been minimized.

Owner

cardoe commented Aug 26, 2018

I replaced your first two commits with a combined version that achieves what you set out to do.

license = license.as_str().split('/').map(|f| f.trim()).collect::<Vec<&str>>().join(" | ");
} else if license.contains("AND") || license.contains("OR") {
license = license.replace("AND", "&&");
license = license.replace("OR", "||");

This comment has been minimized.

@cardoe

cardoe Aug 26, 2018

Owner

This isn't actually ebuild format. ebuilds have the following syntax:

LICENSE="GPL-3+ LGPL-3+ || ( GPL-3+ libgcc libstdc++ ) FDL-1.2+"

Where this is GPL-3+ AND LGPL-3+ AND ( GPL-3+ OR libgcc OR libstdc++ ) AND FDL-1.2+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment