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

Address autobins warning. #5473

Merged
merged 1 commit into from
May 3, 2018
Merged

Address autobins warning. #5473

merged 1 commit into from
May 3, 2018

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented May 3, 2018

Removes the large warning displayed if you build Cargo with itself (but adds a warning if you build with an older Cargo). Not sure which approach you prefer to address it, or if you want to do this later.

@rust-highfive
Copy link

r? @matklad

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

@alexcrichton
Copy link
Member

Awesome thanks! Was about to send a PR for this too :)

Could this actually shuffle the files around to match Cargo's conventions instead of setting autobins though?

@matklad
Copy link
Member

matklad commented May 3, 2018

Could this actually shuffle the files around to match Cargo's conventions instead of setting autobins though?

To elaborate, I think we want

src/
  bin/
    cargo/
      main.rs # former cargo.rs
      cli.rs
       ....

@matklad
Copy link
Member

matklad commented May 3, 2018

Yea, I just realized that this will cause a lot of test failures when run with an older version.

Hm, perhaps we then should hold off issuing the warning until we have a stable Cargo which supports autobins = false?

That way, projects, which use both nightly and stable, could fix the warning without much of a hassle. On the other hand, the fix of "reshuffle directory structure so that conventions just work" seems to be better anyway.

@matklad
Copy link
Member

matklad commented May 3, 2018

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented May 3, 2018

📌 Commit 4224463 has been approved by matklad

@ehuss
Copy link
Contributor Author

ehuss commented May 3, 2018

Hm, perhaps we then should hold off issuing the warning

I was mistaken in my previous comment. However, I do foresee this affecting a lot of projects (I've already seen it in the handful that I've tested with). I agree the renaming approach is much better.

@bors
Copy link
Collaborator

bors commented May 3, 2018

☔ The latest upstream changes (presumably #5468) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor Author

ehuss commented May 3, 2018

Hm, I don't fully understand the error. It looks like the rustc error message got interleaved with cargo's output:

Compiling error: Unrecognized option: 'edition'
foo v0.0.1 (...)

Where that should say:

Compiling foo v0.0.1 (...)
Running ...
error: Unrecognized option: 'edition'

Is that a known issue?

@alexcrichton
Copy link
Member

@bors: r+

Hm that looks like a werid spurious error, somehow a race but I'm not entirely sure how?

@bors
Copy link
Collaborator

bors commented May 3, 2018

📌 Commit 795f69b has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented May 3, 2018

⌛ Testing commit 795f69b with merge af3f1cd...

bors added a commit that referenced this pull request May 3, 2018
Address autobins warning.

Removes the large warning displayed if you build Cargo with itself (but adds a warning if you build with an older Cargo).  Not sure which approach you prefer to address it, or if you want to do this later.
@bors
Copy link
Collaborator

bors commented May 3, 2018

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

@dwijnand
Copy link
Member

dwijnand commented May 4, 2018

Just saw this PR (because of the merge conflict fallout it's had), and wanted to say: feel free to ping me if tweaks are required to autobins (and friends) - I'm going to be AFK for a week, but I'm particularly happy to try to help around changes I've made (and therefore code paths I've somewhat learnt).

@ehuss ehuss added this to the 1.27.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.

6 participants