Infer multi-file binaries like `src/bin/server/main.rs` by convention #4214

msehnout · 2017-06-23T15:41:51Z

This feature is described in issue #4086

rust-highfive · 2017-06-23T15:41:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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. Due to 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.

matklad · 2017-06-24T14:07:18Z

src/cargo/util/toml.rs

+            }).map(|i| {
+            i.path().join("main.rs")
+            // Filter only directories where main.rs is present
+        }).filter(|f| {


Looks like indentation is off here. I would format this as

fn try_add_mains_from_dirs(files: &mut Vec<PathBuf>, root: PathBuf) { if let Ok(new) = fs::read_dir(&root) { let new: Vec<PathBuf> = new.filter_map(|i| i.ok()) // Filter only directories .filter(|i| { i.file_type().map(|f| f.is_dir()).unwrap_or(false) // Convert DirEntry into PathBuf and append "main.rs" }) .map(|i| { i.path().join("main.rs") // Filter only directories where main.rs is present }) .filter(|f| { f.as_path() .exists() }).collect(); files.extend(new); } }

matklad · 2017-06-24T14:15:57Z

Awesome, @msehnout ! Could you add a test from the issue? This one:

src/
  bin/
    foo.rs
    foo/
      main.rs

It should be an error unless there are explicit [bin] sections in Cargo.toml with explicit paths.

Also, I think there's unfortunately one more code path that needs to be modified here. With the current implantation, I think that the following test will fail:

Cargo.toml:

[package]
name = "foo"

[[bin]]
name = "bar"
# Note, no `path` key!

file layout:

src/
  bin/
    bar/
      main.rs

What happens here (at least, this is what I think happens here, the logic is quite complicated :) ) is that Cargo won't use implicit targets from layout if there are some explicitly defined bins in the manifest. However, one can omit the path key in the [[bin]], and then Cargo will infer the path automatically. This happens in inferred_bin_path function, which I think needs to be modified for that test case to work.

That is, there's an implicit contract between the inferred_bin_targets and inferred_bin_path that if the first infers the name N given the path P, the second one should infer the path P given the name N. I wonder if we can remove this duplication here somehow...

matklad · 2017-06-24T14:20:25Z

tests/build.rs

+    assert_that(&p.bin("bar2"), existing_file());
+    assert_that(&p.bin("bar3"), existing_file());
+    assert_that(&p.bin("bar4"), existing_file());
+    assert_that(&p.bin("main"), existing_file());


Hm, I don't think we should implicitly name binary main... Does it happen with stable Cargo as well? Looks like a bug to me!

I'm using stable cargo and it creates a binary called "main" when there is src/bin/main.rs

msehnout · 2017-06-26T12:09:08Z

Thanks for the review. I'll submit changes soon. :-)

msehnout · 2017-06-27T17:56:39Z

I tried to modify inferred_bin_path as well and also added two new tests. But there are still some questions, I mention them in the code here:
https://github.com/msehnout/cargo/blob/5d12c5b4966fa10011a6f6a559e7a73531d535b8/src/cargo/util/toml.rs#L535
and here:
https://github.com/msehnout/cargo/blob/5d12c5b4966fa10011a6f6a559e7a73531d535b8/tests/build.rs#L3313

matklad · 2017-06-27T19:07:56Z

src/cargo/util/toml.rs

+                    if parent.ends_with("src/bin") {
+                        // This would always return name "main"
+                        // Fixme: Is this what we want? based on what @matklad said, I don't think so
+                        // bin.file_stem().and_then(|s| s.to_str()).map(|f| f.to_string())


This is preexisting behavior, so we should leave it as is to preserve backwards compatibility. And I am not actually sure that this is wrong in the first place, it's just curious :)

matklad · 2017-06-27T19:10:39Z

tests/build.rs

+        .file("src/bin/foo/main.rs", "fn main() {}");
+
+    // TODO: This should output the error from toml.rs:756
+    assert_that(p.cargo_process("build"), execs().with_status(101));


That's right! You could use with_stderr_contains to check the error message.

msehnout · 2017-06-28T12:58:26Z

It seems to work finally. Do you have any more comments regarding the code?

matklad

Ok, I believe the logic is quite correct and tests are thorough!

However, I think we've missed the most important part here: it's probably a good idea to document this new convention :)

The relevant docs are here: https://github.com/rust-lang/cargo/blob/64c3217eb459da25df9f9a9818b7775b4387ae8c/src/doc/manifest.md#the-project-layout.

We should tell that multiline binaries go to src/bin/foo/main.rs by convention.

matklad · 2017-06-28T19:49:56Z

src/cargo/util/toml.rs

@@ -505,7 +525,24 @@ fn inferred_bin_targets(name: &str, layout: &Layout) -> Vec<TomlTarget> {
                      *bin == layout.root.join("src").join("main.rs") {
            Some(name.to_string())
        } else {
-            bin.file_stem().and_then(|s| s.to_str()).map(|f| f.to_string())
+            // bin is either a source file or a directory with main.rs inside.
+            if bin.ends_with("main.rs") {


Let's change this to bin.ends_with("main.rs") && !bin.ends_with("src/bin/main.rs") so that we one if inside.

matklad · 2017-06-28T19:50:19Z

src/cargo/util/toml.rs

@@ -1463,6 +1500,12 @@ fn inferred_bin_path(bin: &TomlBinTarget,
            return path.to_path_buf()
        }

+        // check for the case where src/bin/foo/main.rs is present
+        let path = Path::new("src").join("bin").join(bin.name()).join(&format!("main.rs"));


The last bit could be .join("main.rs").

matklad · 2017-06-28T19:50:39Z

src/cargo/util/toml.rs

@@ -1472,6 +1515,12 @@ fn inferred_bin_path(bin: &TomlBinTarget,
        return path.to_path_buf()
    }

+    // we can also have src/bin/foo/main.rs, but the former one is preferred
+    let path = Path::new("src").join("bin").join(bin.name()).join(&format!("main.rs"));


matklad · 2017-06-29T07:56:41Z

LGTM! @alexcrichton do you want to add something else here?

alexcrichton · 2017-06-30T05:29:28Z

@bors: r=matklad

bors · 2017-06-30T05:29:28Z

📌 Commit f08a9d3 has been approved by matklad

bors · 2017-06-30T05:29:35Z

⌛ Testing commit f08a9d3 with merge 1ee4edb...

Infer multi-file binaries like `src/bin/server/main.rs` by convention This feature is described in issue #4086

bors · 2017-06-30T05:49:32Z

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 1ee4edb to master...

Infer multi-file binaries like src/bin/server/main.rs by convention

10661f3

rust-highfive assigned matklad Jun 23, 2017

matklad reviewed Jun 24, 2017

View reviewed changes

Infer paths as well as names for binaries

5d12c5b

matklad reviewed Jun 27, 2017

View reviewed changes

"main" can be valid name for binary again, improve test

1a08d2c

matklad reviewed Jun 28, 2017

View reviewed changes

matklad added the relnotes Release-note worthy label Jun 28, 2017

Martin Sehnoutka added 2 commits June 29, 2017 09:52

Prevent unnecessary heap allocations and simplify condition

8833f3d

Add documentation for multi-file binary convention

f08a9d3

bors added a commit that referenced this pull request Jun 30, 2017

Auto merge of #4214 - msehnout:master, r=matklad

1ee4edb

Infer multi-file binaries like `src/bin/server/main.rs` by convention This feature is described in issue #4086

bors merged commit f08a9d3 into rust-lang:master Jun 30, 2017

bors mentioned this pull request Jun 30, 2017

Stable metadata hashes across workspaces #3611

Merged

crumblingstatue mentioned this pull request Jul 5, 2017

Infer multi-file binaries like src/bin/server/main.rs by convention #4086

Closed

ehuss added this to the 1.20.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infer multi-file binaries like `src/bin/server/main.rs` by convention #4214

Infer multi-file binaries like `src/bin/server/main.rs` by convention #4214

msehnout commented Jun 23, 2017

rust-highfive commented Jun 23, 2017

matklad Jun 24, 2017

matklad commented Jun 24, 2017

matklad Jun 24, 2017

msehnout Jun 26, 2017

msehnout commented Jun 26, 2017

msehnout commented Jun 27, 2017

matklad Jun 27, 2017

matklad Jun 27, 2017

msehnout commented Jun 28, 2017

matklad left a comment

matklad Jun 28, 2017

matklad Jun 28, 2017

matklad Jun 28, 2017

matklad commented Jun 29, 2017

alexcrichton commented Jun 30, 2017

bors commented Jun 30, 2017

bors commented Jun 30, 2017

bors commented Jun 30, 2017

Infer multi-file binaries like src/bin/server/main.rs by convention #4214

Infer multi-file binaries like src/bin/server/main.rs by convention #4214

Conversation

msehnout commented Jun 23, 2017

rust-highfive commented Jun 23, 2017

Choose a reason for hiding this comment

matklad commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msehnout commented Jun 26, 2017

msehnout commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msehnout commented Jun 28, 2017

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Jun 29, 2017

alexcrichton commented Jun 30, 2017

bors commented Jun 30, 2017

bors commented Jun 30, 2017

bors commented Jun 30, 2017

Infer multi-file binaries like `src/bin/server/main.rs` by convention #4214

Infer multi-file binaries like `src/bin/server/main.rs` by convention #4214