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

Rename main thread from "<main>" to "main". #33803

Merged
merged 2 commits into from
Jun 4, 2016
Merged

Rename main thread from "<main>" to "main". #33803

merged 2 commits into from
Jun 4, 2016

Conversation

WiSaGaN
Copy link
Contributor

@WiSaGaN WiSaGaN commented May 23, 2016

Fix issue #33789

We may need to discuss whether this counts as a breaking change since code may check the main thread name against "

". Discussion is in #33789

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

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

@@ -221,15 +221,15 @@ If you add a main function that calls `diverges()` and run it, you’ll get
some output that looks like this:

```text
thread ‘<main>’ panicked at ‘This function never returns!’, hello.rs:2
thread ‘main’ panicked at ‘This function never returns!’, hello.rs:2
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this change specifically, but it looks like some smart quotes snuck into this line.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 23, 2016
@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label May 24, 2016
@alexcrichton
Copy link
Member

The libs team discussed this during triage the other day and there wasn't necessarily a strong consensus either way, but there were some who did want this change.

I did some digging as I was curious about the historical context here. The name <main> was apparently introduced way back when in #10204 to close out #10073. It seems that the name <main> was just based off the precedent set by <unnamed>. That was in turn introduced in 2e6dc16 to handle #2891 and apparently even started out by calling the main task "main"!

Given all that I think there's no reason that this is the way it is, it's just carrying pieces forward.

I'm gonna go ahead and r+ as we're early in the next cycle, but @brson I suspect will surely have an opinion on this as well (he's currently out on vacation), so we may revisit in the next few weeks as well.

Thanks for the PR @WiSaGaN!

@bors: r+ 226bcdf

@bors
Copy link
Contributor

bors commented May 28, 2016

⌛ Testing commit 226bcdf with merge ffa6a90...

@bors
Copy link
Contributor

bors commented May 28, 2016

💔 Test failed - auto-win-msvc-64-cargotest

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented May 28, 2016

There are two tests in cargo that assert against '

'. I have created pull request 2747 in cargo.
But I guess test failure between merging this pull request and cargo-2747 is not avoidable?

bors added a commit to rust-lang/cargo that referenced this pull request May 31, 2016
Rename main thread from '<main>' to 'main'.

This pull request resolves the test failure in rust pull request 33803
rust-lang/rust#33803
@brson
Copy link
Contributor

brson commented May 31, 2016

sgtm

@WiSaGaN to get cargo to pass you'll probably have to weaken the cargo tests so they don't care about the exact text; alternately disable the tests in cargo temporarily, land this PR, then reenable the tests in cargo with the new error string.

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Jun 1, 2016

@brson OK. Cargo test has been updated with the check compatible to this change. I am trying to get buildbot to test the branch again with the new cargo test.
@bors retry

@alexcrichton
Copy link
Member

Ah only those with r+ privs can currently approve PRs (or retry them). I think this'll need to update the rev of cargo in cargotest, however, or otherwise the new changes won't get pulled in by default.

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Jun 2, 2016

@alexcrichton Is there a policy of updating of cargo in cargotest? Or I can upgrade it as another commit in this pull-request?

@alexcrichton
Copy link
Member

Ah you'll have to upgrade it in this PR to land it, so it's fine to just roll it into here. Either as a separate commit or as one of the previous is fine.

@WiSaGaN
Copy link
Contributor Author

WiSaGaN commented Jun 3, 2016

There is some problem in CI preventing the update.

@alexcrichton
Copy link
Member

@bors: r+ f67c4bb

Ah yeah travis is having some problems recently, should be fixed on master I believe though.

@bors
Copy link
Contributor

bors commented Jun 4, 2016

⌛ Testing commit f67c4bb with merge c81c750...

bors added a commit that referenced this pull request Jun 4, 2016
…hton

Rename main thread from "<main>" to "main".

Fix issue #33789

We may need to discuss whether this counts as a breaking change since code may check the main thread name against "\<main\>". Discussion is in #33789
@bors bors merged commit f67c4bb into rust-lang:master Jun 4, 2016
@WiSaGaN WiSaGaN deleted the feature/rename-main-thread branch June 4, 2016 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants