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

Fix rust-lang/rust 40955 #3898

Merged
merged 2 commits into from
Apr 6, 2017
Merged

Fix rust-lang/rust 40955 #3898

merged 2 commits into from
Apr 6, 2017

Conversation

jmatraszek
Copy link
Contributor

@jmatraszek jmatraszek commented Apr 4, 2017

Proposed fix for rust-lang/rust#40955.

I could also work on adding some additional tests, so we have all cases covered and automatically tested.

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

}

// here we have multiple bins, so they are expected to be located inside src/bin
Path::new("src").join("bin").join(&format!("{}.rs", bin.name())).to_path_buf()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcrichton not sure about this, maybe we should support multiple bin targets (for example: foo and bar) in src/foo.rs and src.bar.rs... but what should we do with src/main.rs?

Copy link
Member

Choose a reason for hiding this comment

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

Ah so the intention was that if you have multiple binaries they're all in src/bin/*.rs and if you have one binary it has a few possible locations it could be at, but only one or the other. We didn't intend to support src/main.rs as well as src/bin/*.rs (at least not with automatic inference).

In that sense this looks good to me, thanks!

@alexcrichton
Copy link
Member

Looks great, thanks for tracking this down @jmatraszek!

Looks like there may be a tidy error, but otherwise did you want to add some more tests? I'm ok either way.

@jmatraszek
Copy link
Contributor Author

Hi @alexcrichton,
I would love to add some tests, but the problem is that lately I am extremely busy with my professional job and I just simply do not have time. Maybe will be able to work on the tests on the weekend, so I do not know if you want to wait.

I do not suspect any error right now, just wanted to increase the test coverage for all the cases.

@alexcrichton
Copy link
Member

No worries @jmatraszek! I'll fix up the tidy error and r+ this.

Thanks so much for taking time to investigate this though! Feel free to let me know in the future if you're too busy, it's totally ok to punt it back to me :)

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Apr 5, 2017

📌 Commit 2c9184d has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Apr 5, 2017

⌛ Testing commit 2c9184d with merge c416fb6...

bors added a commit that referenced this pull request Apr 5, 2017
Fix rust-lang/rust 40955

Proposed fix for rust-lang/rust#40955.

I could also work on adding some additional tests, so we have all cases covered and automatically tested.
@bors
Copy link
Collaborator

bors commented Apr 6, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing c416fb6 to master...

@bors bors merged commit 2c9184d into rust-lang:master Apr 6, 2017
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 8, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 12, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 12, 2017
TimNN added a commit to TimNN/rust that referenced this pull request Apr 12, 2017
@ehuss ehuss added this to the 1.18.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants