Implement `cargo init` #2081

vi · Oct 26, 2015

Implement cargo init command and appropriate tests ( #21).

Features:

Working like cargo new if there are no files in current directory
Auto-detection of --bin
Auto-detection of already existing VSC and appending to respecive ignore file
Appending of appropriate [lib] or [[bin]] section to Cargo.toml in case of some non-standard source locations

Concerns:

I'm not experienced in Rust + lazy => code looks poorer compared to the rest Cargo code
The test don't cover 100% of functions
Project consisting of both binary and library is not handled
Many deviations from previously proposed algorithm

rust-highfive · Oct 26, 2015

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wycats (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

vi · Oct 26, 2015

Assignee for my previous attempt was @alexcrichton, but I don't know how to specify assignee when doing pull request here.

Update: found:

r? @alexcrichton

alexcrichton · Oct 28, 2015

Nice work @vi!

I'll see if I can get around to reviewing this tomorrow

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/bin/init.rs

+Create a new cargo package in current directory
+
+Usage:
+    cargo init [options]


Perhaps this could take an optional argument of a directory to initialize as well?

I'm not sure how do I make optional positional arguments here. Naive attempt failed.

Ah I think it's possible with [<optional-arg>]

Found out: the main thing that it's a special library (Docopt) is in use and the usage string matters.

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/ops/cargo_new.rs

-        Some(name) => name,
+fn get_name<'a>(path: &'a Path, opts: &'a NewOptions, config: &Config) -> CargoResult<&'a str> {
+    match opts.name {
+        Some(name) => Ok(name),


This may be a little more nicely formatted as:

if let Some(name) = opts.name { return Ok(name) } // rest ...

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/util/paths.rs

+    metadata(&path).map(|x| x.is_file()).unwrap_or(false)
+}
+
+pub fn directory_already_exists(path: &Path) -> bool {


This may be best named is_dir to mirror what the standard library provides

It may be confused with stdlib's is_dir then, suprising readers with unusually simple signature.

Ah but that's the point :)

(same for is_file above)

(ping on these names)

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/util/paths.rs

+    })
+}
+
+pub fn write_if_not_exists(path: &Path, contents: &[u8]) -> CargoResult<()> {


This helper function seems a little dangerous as it doesn't allow distinguishing the case where a file existed or the file was successfully written, perhaps the relevant locations this is called could just have guards on paths::is_file?

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/ops/cargo_new.rs

    mk(config, &path, name, &opts).chain_error(|| {
        human(format!("Failed to create project `{}` at `{}`",
                      name, path.display()))
    })
 }

+pub fn init(opts: NewOptions, config: &Config) -> CargoResult<()> {


Currently other commands all take the struct of options via a reference, so perhaps that could happen here as well?

I have just copied the signature of pub fn new, which accepts ownership of its options.

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/ops/cargo_new.rs

@@ -17,11 +17,13 @@ use toml;
 #[derive(Clone, Copy, Debug, PartialEq)]
 pub enum VersionControl { Git, Hg, NoVcs }

+#[derive(Clone, Debug, PartialEq)]


Ideally these extra derive modes wouldn't be necessary, how come they were added?

I needed Clone to be able to pass changed options (auto-detected bin, for example) to mk, so duplicated VersionControl's derive line, then removed incompatible Copy, thinking that Debug and PartialEq won't be amiss in any case when they can be implemented.

Should we economize derivations?

Yeah I suspect the changes below would remove the need for Clone, and otherwise I personally prefer to minimize the number of derivations needed to reduce codegen/compile time.

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/ops/cargo_new.rs

+        // if none exists, maybe create git, like in `cargo new`
+
+        if num_detected_vsces > 1 {
+            return Err(human("Both .git and .hg exist. I don't know what to choose."));


Hm this error seems kinda odd, perhaps cargo init could just not do anything VCS-related if no VCS was found? If at least one was found it could just skip that step.

It is the case when multiple VCS-es found, which I expect to be rare and confusing. If none VCS-es found and user hasn't specified any option, it follows what caro new does (i.e. initialize Git, unless it's already inside some another VSC).

If any existing VCS is detected though, couldn't cargo just act as if --vcs none was specified?

(e.g. it just ignores taking those actions)

No, I expect git init && cargo init to also fill in .gitignore.

Actually git init && hg init && cargo init could have been adding both .gitignore and .hgignore, but that (like with multiple source files) complexifies things a bit.

Ah ok that's a good point, I forgot about filling in the ignore files! In that case this error can be a little more targeted than "I don't know what to choose" and could instead mention that with multiple VCS solutions it's not clear which ignore file to fill out.

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/ops/cargo_new.rs

+
+fn detect_source_path_and_type<'a : 'b, 'b>(project_path : &Path, 
+                                            project_name: &str, 
+                                            opts2: &'b mut NewOptions) -> CargoResult<()> {


This argument should probably be called opts rather than opts2, and similarly if the two other arguments are renamed down below they can probably just have those names for the arguments

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/ops/cargo_new.rs

 pub struct NewOptions<'a> {
    pub version_control: Option<VersionControl>,
    pub bin: bool,
    pub path: &'a str,
    pub name: Option<&'a str>,
+    pub sourcefile_relative_path : Option<String>,


This seems a little odd as an option here as it's calculated instead of ever passed in, perhaps this could be stored elsewhere?

In this case mk should accept not NewOptions, but something else. Shall I create special private struct like MkOptions and convert from NewOptions to MkOptions both in new and in init?

Yeah that seems plausible, it could also just be an extra parameter to mk or could be attached to a MkContext which has a method doit or something like that.

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/ops/cargo_new.rs

+    }
+    fn autodetect_bin_file(p: &Path) -> CargoResult<bool> {
+        let mut content = String::new();
+        try!(File::open(p).and_then(|mut x| x.read_to_string(&mut content)));


I think there's a paths::read utility function which should help doing this.

alexcrichton · Nov 2, 2015

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton · Nov 2, 2015

src/cargo/ops/cargo_new.rs

+
+        }
+        opts2.sourcefile_relative_path = Some(format!("{}.rs", name));
+        found_source_files += 1;


This may not be quite the logic we want, this means that if you have src/main.rs as well as src/foo.rs that you'll get an error about multiple source files, right? The latter may just be the library used by the first though.

vi · Nov 2, 2015

Shall I try to improve things according to line comments or there needs to be major redesign of the feature?

alexcrichton · Nov 2, 2015

Nah I think this is definitely along the right lines, so feel free to make changes in this PR!

vi · Nov 2, 2015

How should the {both lib.rs and main.rs exist} situation be resolved?

Ideally Cargo should create a project file with both [lib] and [[bit]] sections, but that makes edits more substantial (multiple paths to source files, bin can't be just a boolean anymore).

Is support of bin+lib project in cargo init a valid reason to make it complexier? Is it a rare or rather typical case?

alexcrichton · Nov 3, 2015

To me it just seemed a little odd to explicitly deny as it's relatively common. It may require some tweaks of how the implementation goes, but in theory it shouldn't be too onerous I think.

vi · Nov 4, 2015

Implemented updated cargo init:

Multi-target projects are now supported. Error may be thrown now only when it thinks that there are multiple libraries in the project (i.e. both src/lib.rs and lib.rs exist, or lib.rs and foo.rs (without main));
detect_source_paths_and_types is refactored;
Most minor issues described in line comments resolved.

alexcrichton · Nov 5, 2015

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton · Nov 5, 2015

src/cargo/util/paths.rs

+pub fn append(path: &Path, contents: &[u8]) -> CargoResult<()> {
+    (|| -> CargoResult<()> {
+        let mut f = try!(
+                    OpenOptions::new()


This indentation seems a bit off, the OpenOptions should come right after the ( of the try!(

alexcrichton · Nov 5, 2015

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton · Nov 5, 2015

src/cargo/ops/cargo_new.rs

@@ -1,5 +1,5 @@
 use std::env;
-use std::fs;
+use std::fs::{self};


This probably doesn't need to be changed if it's just importing self

alexcrichton · Nov 5, 2015

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton · Nov 5, 2015

src/cargo/ops/cargo_new.rs

+            H::Bin => 
+                SourceFileInformation { relative_path: pp, bin: true },
+            H::Lib => 
+                SourceFileInformation { relative_path: pp, bin: false },


Stylistically in Cargo a multi-line match arm has braces around it

alexcrichton · Nov 5, 2015

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton · Nov 5, 2015

src/cargo/ops/cargo_new.rs

+
+    for i in tests {
+        let pp = i.proposed_path;
+        if ! paths::file_already_exists(&path.join(pp.clone())) {


You can avoid the .clone() here by just passing &pp, and there's an extra space after the !

alexcrichton · Nov 5, 2015

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton · Nov 5, 2015

src/cargo/ops/cargo_new.rs

+    Ok(())
+}
+
+fn detect_source_paths_and_types<'a : 'b, 'b>(project_path : &'a Path, 


Are these lifetime annotations necessary? If they're not being returned they can probably be elided

alexcrichton · Nov 5, 2015

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton · Nov 5, 2015

src/cargo/ops/cargo_new.rs

+    Ok(())
+}
+
+fn plan_new_source_file(bin: bool) -> SourceFileInformation {


s/plan/plain/ ?

No, it something around "add creation of new source file to the queue" or "get default SourceFileInformation depending on binary-ness" or "schedule creation of a new source file".

alexcrichton · Jan 20, 2016

(sorry it's being a bit slow...)

alexcrichton · Jan 23, 2016

Thanks for being patient @vi! I talked with @wycats today and we're both quite comfortable moving forward with this. The only lingering thing we thought of is could you tweak the error message for when Cargo.toml already exists? Basically just tailor it to explicitly say "cargo init cannot be run on existing Cargo projects".

vi · Jan 23, 2016

Done + rebased yet again.

alexcrichton · Jan 23, 2016

@bors: r+ 5e62d85

Awesome, thanks again @vi!

bors · Jan 23, 2016

⌛️ Testing commit 5e62d85 with merge cc192a3...

bors · Jan 23, 2016

💔 Test failed - cargo-linux-64

vi · Jan 23, 2016

This test differs from cargo new's test by explicitly not using VCS, hence author name and email don't get extracted from Git and it tries to extract it from system instead, which fails if one unset USER. This function is common in new and init and I expect cargo new to also fail

I can reproduce the failure with

( unset USER; cargo test test_cargo_init )

but not with

( USER= ; cargo test test_cargo_init )

What shall be fixed? Testing tool or cargo_new.rs (which can provide some default unknown author/email)

Update: I see tests do something with .env("USER", now I understand this is probably for reason.

vi · Jan 23, 2016

Fixed the test to have .evn("USER","foo") like other test cases.

alexcrichton · Jan 24, 2016

@bors: r+ 800172f

bors · Jan 24, 2016

⌛️ Testing commit 800172f with merge f052d99...

bors · Jan 24, 2016

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64

WiSaGaN · Feb 17, 2016

Any reason why cargo new --bin adds Cargo.lock to .gitignore?

ticki · Feb 17, 2016

@WiSaGaN Because Cargo.lock is supposed to be a local file.

steveklabnik · Feb 17, 2016

It should be in the ignore for libraries, but not for binaries.

On Feb 17, 2016, 09:29 -0500, Tickinotifications@github.com, wrote:

@WiSaGaN(https://github.com/WiSaGaN)BecauseCargo.lockis supposed to be a local file.

—
Reply to this email directly orview it on GitHub(#2081 (comment)).

vi · Feb 17, 2016

Is there any reason to include Cargo.lock file in Git repository for binary projects?

Better extra entry in gitignore than missing entry. Project may be recrafted into a library after creation...

steveklabnik · Feb 17, 2016

Yes, for repeatable builds. There's a FAQ entry about this distinction.

On Feb 17, 2016, 09:59 -0500, Vitaly Shukelanotifications@github.com, wrote:

Is there any reason to includeCargo.lockfile in Git repository for binary projects?

Better extra entry in gitignore than missing entry. Project may be recrafted into a library after creation...

—
Reply to this email directly orview it on GitHub(#2081 (comment)).

vi · Feb 17, 2016

Shall I implement again bin/lib distinction for ignore file generation?

Shall the lock file be checked into Git automatically by cargo new/init (in addition to not being added to gitignore) for bin projects?

WiSaGaN · Feb 17, 2016

@vi , yes, please.
Restoring the original should be fine:
https://github.com/rust-lang/cargo/pull/2081/files#diff-149dd4362a3b0dc13b113762713119dfL107

vi · Feb 17, 2016

It's not that simple.

Restored the function in a proper way: #2390

alexcrichton · Feb 17, 2016

@WiSaGaN, @vi, oh oops! I missed that in review by accident, yes it's intentional that bin projects do not ignore Cargo.lock by default

vi · Feb 17, 2016

I remember wondering about whether the condition around adding Cargo.lock is really necessary and recieving something like "Just add it to ignore" reply.

rust-highfive assigned wycats Oct 26, 2015

vi changed the title ~~Implement `cargo init` (#21)~~ Implement `cargo init` Oct 26, 2015

alexcrichton assigned alexcrichton and unassigned wycats Oct 28, 2015

alexcrichton referenced this pull request Nov 5, 2015
Closed
Cargo new in empty dir #2008

alexcrichton added the relnotes label Jan 23, 2016

vi force-pushed the vi:cargo_init branch from ebd2186 to 5e62d85 Jan 23, 2016

vi force-pushed the vi:cargo_init branch from 5e62d85 to 800172f Jan 23, 2016

bors merged commit 800172f into rust-lang:master Jan 24, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors referenced this pull request Jan 24, 2016
Closed
Allow licenses to be specified during `cargo new` #2013

rasendubi referenced this pull request Jan 25, 2016
Closed
cargo-init would be super nice #21

rust-lang/cargo

Join GitHub today

Implement `cargo init` #2081

Conversation

vi commented Oct 26, 2015

rust-highfive assigned wycats Oct 26, 2015

This comment has been minimized.

rust-highfive commented Oct 26, 2015

This comment has been minimized.

vi commented Oct 26, 2015

vi changed the title Implement `cargo init` (#21) Implement `cargo init` Oct 26, 2015

This comment has been minimized.

alexcrichton commented Oct 28, 2015

alexcrichton assigned alexcrichton and unassigned wycats Oct 28, 2015

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 2, 2015 View changes

This comment has been minimized.

This comment has been minimized.

vi commented Nov 2, 2015

This comment has been minimized.

alexcrichton commented Nov 2, 2015

This comment has been minimized.

vi commented Nov 2, 2015

This comment has been minimized.

alexcrichton commented Nov 3, 2015

This comment has been minimized.

vi commented Nov 4, 2015

alexcrichton referenced this pull request Nov 5, 2015

alexcrichton reviewed Nov 5, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 5, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 5, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 5, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 5, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Nov 5, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Jan 20, 2016

This comment has been minimized.

alexcrichton commented Jan 23, 2016

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 2, 2015

View changes

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton reviewed Nov 5, 2015

View changes

alexcrichton reviewed Nov 5, 2015

View changes

vi force-pushed the vi:cargo_init branch from `ebd2186` to `5e62d85` Jan 23, 2016

vi force-pushed the vi:cargo_init branch from `5e62d85` to `800172f` Jan 23, 2016

bors merged commit `800172f` into rust-lang:master Jan 24, 2016
3 checks passed