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

respect user choice of lib/bin over heuristics #9522

Merged
merged 5 commits into from
Jun 9, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/bin/cargo/commands/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ pub fn cli() -> App {

pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
let opts = args.new_options(config)?;
ops::init(&opts, config)?;
let project_kind = ops::init(&opts, config)?;
config
.shell()
.status("Created", format!("{} package", opts.kind))?;
.status("Created", format!("{} package", project_kind))?;
Ok(())
}
4 changes: 2 additions & 2 deletions src/bin/cargo/commands/new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub fn cli() -> App {
pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
let opts = args.new_options(config)?;

ops::new(&opts, config)?;
let project_kind = ops::new(&opts, config)?;
let path = args.value_of("path").unwrap();
let package_name = if let Some(name) = args.value_of("name") {
name
Expand All @@ -24,7 +24,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
};
config.shell().status(
"Created",
format!("{} `{}` package", opts.kind, package_name),
format!("{} `{}` package", project_kind, package_name),
)?;
Ok(())
}
85 changes: 66 additions & 19 deletions src/cargo/ops/cargo_new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,25 @@ pub struct NewOptions {
pub enum NewProjectKind {
Bin,
Lib,
Auto,
}

impl NewProjectKind {
fn is_bin(self) -> bool {
self == NewProjectKind::Bin
}

fn is_auto(self) -> bool {
self == NewProjectKind::Auto
}
}

impl fmt::Display for NewProjectKind {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match *self {
NewProjectKind::Bin => "binary (application)",
NewProjectKind::Lib => "library",
NewProjectKind::Auto => "auto-select type",
Aelnor marked this conversation as resolved.
Show resolved Hide resolved
}
.fmt(f)
}
Expand Down Expand Up @@ -109,8 +115,8 @@ impl NewOptions {
let kind = match (bin, lib) {
(true, true) => anyhow::bail!("can't specify both lib and binary outputs"),
(false, true) => NewProjectKind::Lib,
// default to bin
(_, false) => NewProjectKind::Bin,
(true, false) => NewProjectKind::Bin,
(false, false) => NewProjectKind::Auto,
};

let opts = NewOptions {
Expand Down Expand Up @@ -389,7 +395,26 @@ fn plan_new_source_file(bin: bool, package_name: String) -> SourceFileInformatio
}
}

pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> {
fn calculate_new_project_kind(
requested_kind: NewProjectKind,
found_files: &Vec<SourceFileInformation>,
) -> NewProjectKind {
let bin_file = found_files.iter().find(|x| x.bin);

let kind_from_files = if !found_files.is_empty() && bin_file.is_none() {
NewProjectKind::Lib
} else {
NewProjectKind::Bin
};

if requested_kind.is_auto() {
return kind_from_files;
}

requested_kind
}

pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<NewProjectKind> {
let path = &opts.path;
if path.exists() {
anyhow::bail!(
Expand All @@ -399,20 +424,22 @@ pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> {
)
}

let kind = match opts.kind {
NewProjectKind::Bin => NewProjectKind::Bin,
NewProjectKind::Auto => NewProjectKind::Bin,
_ => NewProjectKind::Lib,
};
Aelnor marked this conversation as resolved.
Show resolved Hide resolved
let is_bin = kind.is_bin();

let name = get_name(path, opts)?;
check_name(
name,
opts.name.is_none(),
opts.kind.is_bin(),
&mut config.shell(),
)?;
check_name(name, opts.name.is_none(), is_bin, &mut config.shell())?;

let mkopts = MkOptions {
version_control: opts.version_control,
path,
name,
source_files: vec![plan_new_source_file(opts.kind.is_bin(), name.to_string())],
bin: opts.kind.is_bin(),
bin: is_bin,
edition: opts.edition.as_deref(),
registry: opts.registry.as_deref(),
};
Expand All @@ -424,10 +451,10 @@ pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> {
path.display()
)
})?;
Ok(())
Ok(kind)
}

pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<NewProjectKind> {
// This is here just as a random location to exercise the internal error handling.
if std::env::var_os("__CARGO_TEST_INTERNAL_ERROR").is_some() {
return Err(crate::util::internal("internal error test"));
Expand All @@ -445,14 +472,34 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {

detect_source_paths_and_types(path, name, &mut src_paths_types)?;

let kind = calculate_new_project_kind(opts.kind, &src_paths_types);
let has_bin = kind.is_bin();

if src_paths_types.is_empty() {
src_paths_types.push(plan_new_source_file(opts.kind.is_bin(), name.to_string()));
} else {
// --bin option may be ignored if lib.rs or src/lib.rs present
// Maybe when doing `cargo init --bin` inside a library package stub,
// user may mean "initialize for library, but also add binary target"
src_paths_types.push(plan_new_source_file(has_bin, name.to_string()));
} else if src_paths_types.len() == 1 && !src_paths_types.iter().any(|x| x.bin == has_bin) {
// we've found the only file and it's not the type user wants. Change the type and warn
let file_type = if src_paths_types[0].bin {
NewProjectKind::Bin.to_string()
} else {
NewProjectKind::Lib.to_string()
Aelnor marked this conversation as resolved.
Show resolved Hide resolved
};
config.shell().warn(format!(
"file '{}' seems to be a {} file",
Aelnor marked this conversation as resolved.
Show resolved Hide resolved
src_paths_types[0].relative_path, file_type
))?;
src_paths_types[0].bin = has_bin
} else if src_paths_types.len() > 1 && !has_bin {
// We have found both lib and bin files and the user would like us to treat both as libs
anyhow::bail!(
"cannot have a package with \
multiple libraries, \
found both `{}` and `{}`",
src_paths_types[0].relative_path,
src_paths_types[1].relative_path
)
}
let has_bin = src_paths_types.iter().any(|x| x.bin);

check_name(name, opts.name.is_none(), has_bin, &mut config.shell())?;

let mut version_control = opts.version_control;
Expand Down Expand Up @@ -508,7 +555,7 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
path.display()
)
})?;
Ok(())
Ok(kind)
}

/// IgnoreList
Expand Down
65 changes: 65 additions & 0 deletions tests/testsuite/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,3 +598,68 @@ mod tests {
"#
);
}

#[cargo_test]
fn creates_binary_when_instructed_and_has_lib_file_no_warning() {
let path = paths::root().join("foo");
fs::create_dir(&path).unwrap();
fs::write(path.join("foo.rs"), "fn not_main() {}").unwrap();
cargo_process("init --bin")
.cwd(&path)
.with_stderr(
"[WARNING] file 'foo.rs' seems to be a library file\n[CREATED] binary (application) package"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure in this case we should issue a warning. It means there's no fn main most likely

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning seems fine to me. I imagine the user can figure things out afterwards.

)
.run();

let cargo_toml = fs::read_to_string(path.join("Cargo.toml")).unwrap();
assert!(cargo_toml.contains("[[bin]]"));
assert!(!cargo_toml.contains("[lib]"));
}

#[cargo_test]
fn creates_library_when_instructed_and_has_bin_file() {
let path = paths::root().join("foo");
fs::create_dir(&path).unwrap();
fs::write(path.join("foo.rs"), "fn main() {}").unwrap();
cargo_process("init --lib")
.cwd(&path)
.with_stderr(
"[WARNING] file 'foo.rs' seems to be a binary (application) file\n[CREATED] library package"
Aelnor marked this conversation as resolved.
Show resolved Hide resolved
)
.run();

let cargo_toml = fs::read_to_string(path.join("Cargo.toml")).unwrap();
assert!(!cargo_toml.contains("[[bin]]"));
assert!(cargo_toml.contains("[lib]"));
}

#[cargo_test]
fn creates_binary_when_both_binlib_present() {
let path = paths::root().join("foo");
fs::create_dir(&path).unwrap();
fs::write(path.join("foo.rs"), "fn main() {}").unwrap();
fs::write(path.join("lib.rs"), "fn notmain() {}").unwrap();
cargo_process("init --bin")
.cwd(&path)
.with_stderr("[CREATED] binary (application) package")
.run();

let cargo_toml = fs::read_to_string(path.join("Cargo.toml")).unwrap();
assert!(cargo_toml.contains("[[bin]]"));
assert!(cargo_toml.contains("[lib]"));
}

#[cargo_test]
fn cant_create_library_when_both_binlib_present() {
let path = paths::root().join("foo");
fs::create_dir(&path).unwrap();
fs::write(path.join("foo.rs"), "fn main() {}").unwrap();
fs::write(path.join("lib.rs"), "fn notmain() {}").unwrap();
cargo_process("init --lib")
.cwd(&path)
.with_status(101)
.with_stderr(
"[ERROR] cannot have a package with multiple libraries, found both `foo.rs` and `lib.rs`"
)
.run();
}