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

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

Closed
matklad opened this issue May 22, 2017 · 16 comments · Fixed by #4496
Closed

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

matklad opened this issue May 22, 2017 · 16 comments · Fixed by #4496
Labels
E-easy Experience: Easy

Comments

@matklad
Copy link
Member

matklad commented May 22, 2017

Currently cargo infers binaries from files in src/bin. This works if you binary fits in one file, but if you need more than one file, you have to write custom [[bin]] specification in Cargo.toml.

Looks like a reasonable convention for multifile binary projects is this:

src/
  bin/
    server/
        main.rs
        aux.rs
    client/
        main.rs
        foo.rs

So let's infer src/bin/foo/main.rs as a binary named foo?

This should be mostly backwards compatible change. The only failure case is when someone has src/bin/foo/main.rs which is not a binary, and no [[bin]] sections are present, and this seems unlikely.

A potential edge case is

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

This should be an error.

@alexcrichton
Copy link
Member

Sounds great to me!

@matklad matklad added E-easy Experience: Easy E-help-wanted labels Jun 13, 2017
@matklad
Copy link
Member Author

matklad commented Jun 13, 2017

If anyone wants to fix it, the place to look at is this line

try_add_files(&mut bins, root_path.join("src").join("bin"));

and this function
fn inferred_bin_targets(name: &str, layout: &Layout) -> Vec<TomlTarget> {
.

The tests should probably go into this file: https://github.com/rust-lang/cargo/blob/master/tests/build.rs

@msehnout
Copy link
Contributor

Is anybody working on this?

@alexcrichton
Copy link
Member

@msehnout not that I know of, feel free to take it!

bors added a commit that referenced this issue Jun 30, 2017
Infer multi-file binaries like `src/bin/server/main.rs` by convention

This feature is described in issue #4086
@crumblingstatue
Copy link

#4214 enabled this for src/bin, but how about also enabling this pattern for examples (and maybe integration tests)?

@matklad
Copy link
Member Author

matklad commented Jul 8, 2017

@crumblingstatue that sounds plausible! Right now I am refactoring the implementation of this stuff in #4259 though, so, if you'd like to send a PR, it's better to wait for a while :)

@rwakulszowa
Copy link
Contributor

Hi @matklad, I think I could try to work on this one. The PR you mentioned is merged already. Is there anything I should be aware of before I start?

@matklad
Copy link
Member Author

matklad commented Sep 10, 2017

Awesome @rwakulszowa ! I think toml/targets.rs is the only file that should me modified, besides the test.

@rwakulszowa
Copy link
Contributor

Hey, I have a few questions

  • the discussion here mentions tests and examples. What about banchmarks?
  • is there a nice way to test for existence of test binaries? Examples end up in the examples folder, but tests only produce artifacts in the main folder, decorated with hashes; is there a way to pass a regex / prefix string to the assert_that(..., existing_file())?

@matklad
Copy link
Member Author

matklad commented Sep 13, 2017

the discussion here mentions tests and examples. What about banchmarks?

Yeah, the benchmarks could use the same logic

is there a nice way to test for existence of test binaries?

I would probably check that cargo test --test foo works.

bors added a commit that referenced this issue Sep 18, 2017
Infer targets from subdirectories

Fixes #4086

I still have a few questions:
- should I add some tests for the old behaviour? It isn't really tested at the moment (no tests failed when I broke the implementation); I could refactor the tests to check for both single file and subdirectory inference
- I moved things around, mostly reusing the code from `inferred_bins` - hopefully I didn't break anything, but it won't hurt to double check :)
- Do we have something like servo's `tidy` check for coding style? I'm open for suggestions if something isn't formatted correctly
- Just a general one - should I rebase + squash commits every time I make subsequent changes to cargo?
@sunjay
Copy link
Member

sunjay commented Nov 19, 2017

Did support for examples with a main.rs file get added as part of this or should that be a separate issue?

@rwakulszowa
Copy link
Contributor

rwakulszowa commented Nov 19, 2017 via email

@sunjay
Copy link
Member

sunjay commented Nov 19, 2017

@rwakulszowa Yes, it didn't work. Here's a minimal example:

From the base project created by cargo new --lib <project name>, I added a file examples/dir/main.rs and a file examples/foo.rs.

.
├── Cargo.lock
├── Cargo.toml
├── examples
│   ├── dir
│   │   └── main.rs
│   └── foo.rs
└── src
    └── lib.rs

Both files contain the simple hello world program. When I try to run them, this is what happens:

$ cargo run --example dir
error: no example target named `dir`

Did you mean `foo`?
$ cargo run --example foo
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/examples/foo`
Hello, world!

The flat file worked, but nesting a file within a directory and having a file called "main.rs" did not work.

@rwakulszowa
Copy link
Contributor

rwakulszowa commented Nov 19, 2017 via email

@sunjay
Copy link
Member

sunjay commented Nov 19, 2017

Maybe the feature didn't make it to the official release yet, or some other
changes modified the behavior. I'll take a look.

Yes it seems so. I tried running it with cargo +nightly run --example dir and it worked. Looks like this will probably be out in Rust 1.22 😄

@rwakulszowa
Copy link
Contributor

rwakulszowa commented Nov 19, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Experience: Easy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants